Closed Bug 1467572 Opened 6 years ago Closed 6 years ago

Implement the new design for RDM

Categories

(DevTools :: Responsive Design Mode, enhancement, P3)

enhancement

Tracking

(firefox63 verified)

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: gl, Assigned: gl)

References

(Depends on 2 open bugs, Regressed 1 open bug, )

Details

Attachments

(19 files, 6 obsolete files)

59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
flod
: review+
Details
59 bytes, text/x-review-board-request
rcaliman
: review+
Details
      No description provided.
Re: Part 9. You will see that I had to re-add some css that I removed to keep the device modal looking and behaving the same. Otherwise, I simply got rid of a lot of styles to get us the look I assumed we wanted.

Subsequent patches will get unit tests working again, some more styling for the select menus, and changing the reload dropdown to be a settings button and adjusting the menu it displays.
Comment on attachment 8984796 [details]
Bug 1467572 - Part 1: Move the App component into the component folder.

https://reviewboard.mozilla.org/r/250592/#review257002

Makes sense, thanks!
Attachment #8984796 - Flags: review?(jryans) → review+
Comment on attachment 8984800 [details]
Bug 1467572 - Part 2: Remove the viewport toolbar.

https://reviewboard.mozilla.org/r/250602/#review257064
Attachment #8984800 - Flags: review?(jryans) → review+
Comment on attachment 8984801 [details]
Bug 1467572 - Part 3: Remove the viewport dimension.

https://reviewboard.mozilla.org/r/250604/#review257066

::: devtools/client/responsive.html/components/Viewport.js:70
(Diff revision 1)
> -      ViewportDimension({
> -        viewport,
> -        onChangeSize: onResizeViewport,
> -        onRemoveDeviceAssociation,
> -      }),
>        ResizableViewport({

Maybe we should consider removing this `Viewport` component now that it's only a container for a single component?  Although the `.viewport` class seems good to preserve...  Maybe file a follow up?
Attachment #8984801 - Flags: review?(jryans) → review+
Comment on attachment 8984802 [details]
Bug 1467572 - Part 4: Move the device selector into the global toolbar.

https://reviewboard.mozilla.org/r/250606/#review257080

::: devtools/client/responsive.html/components/App.js:209
(Diff revision 1)
> +    let viewportId = 0;
>  
>      if (viewports.length) {
>        selectedDevice = viewports[0].device;
>        selectedPixelRatio = viewports[0].pixelRatio;
> +      viewportId = viewports[0].id;

Assuming there's some eventual multi-viewport world, toolbars like the device selector would once again have per-viewport placement.  Rather than threading the `viewportId` through all components, I think it's simpler to modify the action functions like `onDeviceChange` in the app component to access the id.  This is similar to how the `Viewport` component used to embed the id in these handlers (though you have just removed those in previous patches here).

What do you think?
Attachment #8984802 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Comment on attachment 8984802 [details]
> Bug 1467572 - Part 4: Move the device selector into the global toolbar.
> 
> https://reviewboard.mozilla.org/r/250606/#review257080
> 
> ::: devtools/client/responsive.html/components/App.js:209
> (Diff revision 1)
> > +    let viewportId = 0;
> >  
> >      if (viewports.length) {
> >        selectedDevice = viewports[0].device;
> >        selectedPixelRatio = viewports[0].pixelRatio;
> > +      viewportId = viewports[0].id;
> 
> Assuming there's some eventual multi-viewport world, toolbars like the
> device selector would once again have per-viewport placement.  Rather than
> threading the `viewportId` through all components, I think it's simpler to
> modify the action functions like `onDeviceChange` in the app component to
> access the id.  This is similar to how the `Viewport` component used to
> embed the id in these handlers (though you have just removed those in
> previous patches here).
> 
> What do you think?

Yes I agree, I will redo that work in this patch.
Product: Firefox → DevTools
Perhaps you have thought or attempted this. I was trying to use the Menu/MenuItem components to get a context menu for the Reload menu component and replace having to write our own context menu and styling it. If this worked, I would've replaced all our <selects> with using ToggleMenu, but it would toggle open the Menu context menu instead. 

Of course, I couldn't get the Menu/MenuItem components to work because it assumes having a toolbox and XUL(?). Would you be possible to look at what would be needed to make the Menu component work in our current document? 

My thinking here would be to avoid writing our own context menu and having to style it since the designs call for some fancier select option menus.
Comment on attachment 8984802 [details]
Bug 1467572 - Part 4: Move the device selector into the global toolbar.

https://reviewboard.mozilla.org/r/250606/#review257100

The mockup seems to add a separator between the devices and the editing option.  Also, the editing option's label has new text of "Edit list...".
Comment on attachment 8984852 [details]
Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar.

https://reviewboard.mozilla.org/r/250654/#review257106

::: devtools/client/responsive.html/components/ViewportDimension.js:56
(Diff revision 1)
>  
> -  validateInput(value) {
> -    let isInvalid = true;
> -
> -    // Check the value is a number and greater than MIN_VIEWPORT_DIMENSION
> -    if (/^\d{3,4}$/.test(value) &&
> +  /**
> +   * Return true if the given value is a number and greater than MIN_VIEWPORT_DIMENSION
> +   * and false otherwise.
> +   */
> +  isInputValid(value) {

This no longer depends on component state, should it move to a standalone function?

::: devtools/client/responsive.html/index.css:392
(Diff revision 1)
> -.viewport-dimension-editable.editing.invalid {
> -  border-bottom: 1px solid #d92215;
>  }
>  
> -.viewport-dimension-input {
> -  background: transparent;
> +.viewport-dimension-input.invalid:focus {
> +  border: 1px solid #d92215;

Is there a relevant variable for this color?
Attachment #8984852 - Flags: review?(jryans) → review+
Comment on attachment 8984857 [details]
Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar.

https://reviewboard.mozilla.org/r/250656/#review257108
Attachment #8984857 - Flags: review?(jryans) → review+
Comment on attachment 8984859 [details]
Bug 1467572 - Part 8: Add proper separators to the global toolbar.

https://reviewboard.mozilla.org/r/250658/#review257110
Attachment #8984859 - Flags: review?(jryans) → review+
Comment on attachment 8984852 [details]
Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar.

https://reviewboard.mozilla.org/r/250654/#review257106

> Is there a relevant variable for this color?

No, but I think we will have to update these colors in a patch I haven't done yet.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> Comment on attachment 8984802 [details]
> Bug 1467572 - Part 4: Move the device selector into the global toolbar.
> 
> https://reviewboard.mozilla.org/r/250606/#review257100
> 
> The mockup seems to add a separator between the devices and the editing
> option.  Also, the editing option's label has new text of "Edit list...".

I think I fixed this in Part 9.
Victoria, any opinions on the styling of the device addition form (it appears after choosing "Manage presets" and clicking "Add Device")?  With Gabriel's work so far, this form feels a bit out of place because it still relies on styling from the old design.
Flags: needinfo?(victoria)
Comment on attachment 8984804 [details]
Bug 1467572 - Part 5: Implement the new style for the global toolbar.

https://reviewboard.mozilla.org/r/250608/#review257084

This seems like it's on the right track, but I'd like to check it again.

::: devtools/client/responsive.html/components/GlobalToolbar.js:70
(Diff revision 2)
>      if (touchSimulation.enabled) {
>        touchButtonClass += " checked";
>      }
>  
>      return dom.header(
> -      {
> +      { id: "global-toolbar" },

All of this "global" naming feels a bit strange now that the viewport toolbar is gone.  What if we rename this component to just `Toolbar`?  It's not very "global" anyway, as it now contains a mish-mash of viewport specific and general controls.  Then we can also remove the `global-` prefix from IDs and classes.

::: devtools/client/responsive.html/index.css:36
(Diff revision 2)
>  
>  :root,
>  input,
>  select,
>  button {
> -  font-size: 11px;
> +  font-size: 12px;

Is there a DevTools variable we can use instead?  If not, should there be?

::: devtools/client/responsive.html/index.css:148
(Diff revision 2)
>  .devtools-toggle-menu .devtools-menu {
>    display: none;
>    flex-direction: column;
>    align-items: start;
>    position: absolute;
>    left: 0;

The reload conditions menu now appears offscreen since the control is on the right end of the bar.  Maybe it's enough to change this to `right: 0`?

Ah, I guess you might be addressing this in a future patch.

::: devtools/client/responsive.html/index.css:185
(Diff revision 2)
>  }
>  
> +.toolbar-dropdown {
> +  background-position-x: right 5px;
> +  border-right: 1px solid var(--theme-splitter-color);
> +  padding-right: 15px;

What happens in RTL?

::: devtools/client/responsive.html/index.css:192
(Diff revision 2)
> +
>  /**
>   * Global Toolbar
>   */
>  
>  #global-toolbar {

If you open the toolbox in bottom dock mode, once the height increases enough to hit the viewport (causing the RDM UI to scroll), the toolbar seems to shrink by several pixels.

::: devtools/client/responsive.html/index.css:192
(Diff revision 2)
> +
>  /**
>   * Global Toolbar
>   */
>  
>  #global-toolbar {

With the dark theme, most toolbar labels are currently getting black text making them hard to read.

::: devtools/client/responsive.html/index.css:196
(Diff revision 2)
>  
>  #global-toolbar {
> -  color: var(--theme-body-color-alt);
> -  border-radius: 2px;
> -  box-shadow: var(--rdm-box-shadow);
> -  margin: 0 0 15px 0;
> +  background-color: var(--theme-toolbar-background);
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +  display: grid;
> +  grid-template-columns: min-content auto max-content;

Why min-content for one but max-content for the other?  It seems like min-content and max-content are equal for both...?
Attachment #8984804 - Flags: review?(jryans) → review-
(In reply to Gabriel [:gl] (ΦωΦ) from comment #24)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> > Comment on attachment 8984802 [details]
> > Bug 1467572 - Part 4: Move the device selector into the global toolbar.
> > 
> > https://reviewboard.mozilla.org/r/250606/#review257100
> > 
> > The mockup seems to add a separator between the devices and the editing
> > option.  Also, the editing option's label has new text of "Edit list...".
> 
> I think I fixed this in Part 9.

It seems like you changed the text away from the mockup...? I am confused now...

I am looking at https://mozilla.invisionapp.com/share/2NK8A5CWVPG#/screens/300479943, are you using a different one?

Currently I don't see a separator added when testing locally.  It might also be good to add another separator as well between "Responsive" and the devices...?
Comment on attachment 8984878 [details]
Bug 1467572 - Part 9 - Style the global toolbar and device modal.

https://reviewboard.mozilla.org/r/250660/#review257102

Thank you for working on this!  Overall, it looks great so far!

In particular, thank you for greatly simplifying the mess of CSS here. :)

r- for now, as I would like to understand what Victoria intends for device adder.

::: devtools/client/locales/en-US/responsive.properties:16
(Diff revision 1)
>  # A good criteria is the language in which you'd find the best
>  # documentation on web development on the web.
>  
> -# LOCALIZATION NOTE (responsive.editDeviceList): option displayed in the device
> +# LOCALIZATION NOTE (responsive.managePresets): option displayed in the device
>  # selector
> -responsive.editDeviceList=Edit list…
> +responsive.managePresets=Manage presets…

Wait, now I am confused...  The mockup seems to say "Edit list..."?

https://mozilla.invisionapp.com/share/2NK8A5CWVPG#/screens/300479943

Are you using a different one?

::: devtools/client/locales/en-US/responsive.properties:37
(Diff revision 1)
>  # LOCALIZATION NOTE (responsive.done): button text in the device list modal
>  responsive.done=Done
>  
> -# LOCALIZATION NOTE (responsive.noDeviceSelected): placeholder text for the
> +# LOCALIZATION NOTE (responsive.responsive): placeholder text for the
>  # device selector
> -responsive.noDeviceSelected=no device selected
> +responsive.responsive=Responsive

Maybe name the string `responsive.reponsiveMode` or something slightly more specific?

::: devtools/client/responsive.html/components/DeviceSelector.js:91
(Diff revision 1)
>            value: "",
>            title: "",
>            disabled: true,
>            hidden: true,
>          },
> -        getStr("responsive.noDeviceSelected")),
> +        getStr("responsive.responsive")),

Maybe add a separator after this?

::: devtools/client/responsive.html/components/DeviceSelector.js:99
(Diff revision 1)
>              key: device.name,
>              value: device.name,
>              title: "",
>            }, device.name);
>          }),
>          dom.option({

The mockup has a separator above this.

::: devtools/client/responsive.html/index.css:12
(Diff revision 1)
>  .theme-light {
>    --rdm-box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26);
>    --submit-button-active-background-color: rgba(0,0,0,0.12);
>    --submit-button-active-color: var(--theme-body-color);
> -  --viewport-color: #999797;
>    --viewport-hover-color: var(--theme-body-color);

Why does only one theme now define hover color?  Should this be removed?

::: devtools/client/responsive.html/index.css:124
(Diff revision 1)
>   * Common background for dropdowns like select and toggle menu
>   */
>  
>  .toolbar-dropdown,
>  .toolbar-dropdown.devtools-button,
>  .toolbar-dropdown.devtools-button:hover:not(:empty):not(:disabled):not(.checked) {

Any chance we can blow away these giant selectors too?

::: devtools/client/responsive.html/index.css:503
(Diff revision 1)
> +  justify-content: center;
> +  transition: all 0.25s ease;
> +}
> +
> +#device-adder label > .viewport-dimension.editing {
> +  border-bottom-color: var(--theme-selection-background);

I think Victoria should clarify what style we really want for this form.  As it is, it no longer seems to match the rest of the UI.
Attachment #8984878 - Flags: review?(jryans) → review-
I did not add (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)
> (In reply to Gabriel [:gl] (ΦωΦ) from comment #24)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> > > Comment on attachment 8984802 [details]
> > > Bug 1467572 - Part 4: Move the device selector into the global toolbar.
> > > 
> > > https://reviewboard.mozilla.org/r/250606/#review257100
> > > 
> > > The mockup seems to add a separator between the devices and the editing
> > > option.  Also, the editing option's label has new text of "Edit list...".
> > 
> > I think I fixed this in Part 9.
> 
> It seems like you changed the text away from the mockup...? I am confused
> now...
> 
> I am looking at
> https://mozilla.invisionapp.com/share/2NK8A5CWVPG#/screens/300479943, are
> you using a different one?
> 
> Currently I don't see a separator added when testing locally.  It might also
> be good to add another separator as well between "Responsive" and the
> devices...?

Sorry, I did not add a separator in the menu. I haven't done anything in regards to those select options. The current focus was on just getting the global toolbar to look like the mockup.
Gonna needinfo you again https://bugzilla.mozilla.org/show_bug.cgi?id=1467572#c18 in case you missed that comment.
Flags: needinfo?(jryans)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #28)
> I was looking at
> https://mozilla.invisionapp.com/d/main#/console/12408235/260632515/preview

Okay.  That is older than the mockup that Victoria linked in the Design Tools meeting (which is the one I mentioned), however there don't seem to be too many differences, other than this text...

Victoria, should the device modal label be "Edit list..." or "Manage presets..."?
(In reply to Gabriel [:gl] (ΦωΦ) from comment #18)
> Perhaps you have thought or attempted this. I was trying to use the
> Menu/MenuItem components to get a context menu for the Reload menu component
> and replace having to write our own context menu and styling it. If this
> worked, I would've replaced all our <selects> with using ToggleMenu, but it
> would toggle open the Menu context menu instead. 
> 
> Of course, I couldn't get the Menu/MenuItem components to work because it
> assumes having a toolbox and XUL(?). Would you be possible to look at what
> would be needed to make the Menu component work in our current document? 
> 
> My thinking here would be to avoid writing our own context menu and having
> to style it since the designs call for some fancier select option menus.

Right, I believe we'd prefer to use the native menus if we could, but the XUL doc requirement makes it hard to do at the moment.  We have bug 1459972 on the RDM side to change to this, but I believe it depends on a bit of platform work to allow them in HTML docs (bug 1466897).

So, for this current bug, you may have to proceed for now with the custom menu.
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #33)
> (In reply to Gabriel [:gl] (ΦωΦ) from comment #18)
> So, for this current bug, you may have to proceed for now with the custom
> menu.

(cue internal monologue - Nooooooooo!!)
Depends on: 1466897
See Also: → 1461522
Sorry about the confusion with the mockups! The one with "Edit List" is the more recent mockup meant for this polish task. (Manage Presets was meant for something more involved, e.g. workspaces). 

As for the Edit panel, I agree it needs work - perhaps it would be best to make a separate patch for this? The main issues I see besides the input fields are the the Save/Close buttons, the need for more visual separation for the add-device section, and lack of scrolling when the browser is too short. Alignment and icon polish would be good too. This might be a good UX contributor project.
Flags: needinfo?(victoria)
I deleted the Invision screen gl was using for sanity's sake - here's the right one for this bug:
public link: https://mozilla.invisionapp.com/share/MZLCCV77DAK
inspect link: https://mozilla.invisionapp.com/d/main#/console/12408235/300479943/inspect
Comment on attachment 8984801 [details]
Bug 1467572 - Part 3: Remove the viewport dimension.

https://reviewboard.mozilla.org/r/250604/#review262962

::: devtools/client/responsive.html/components/Viewport.js:70
(Diff revision 1)
> -      ViewportDimension({
> -        viewport,
> -        onChangeSize: onResizeViewport,
> -        onRemoveDeviceAssociation,
> -      }),
>        ResizableViewport({

Fixed. I removed the Viewport comoponent.
Comment on attachment 8984802 [details]
Bug 1467572 - Part 4: Move the device selector into the global toolbar.

https://reviewboard.mozilla.org/r/250606/#review257080

> Assuming there's some eventual multi-viewport world, toolbars like the device selector would once again have per-viewport placement.  Rather than threading the `viewportId` through all components, I think it's simpler to modify the action functions like `onDeviceChange` in the app component to access the id.  This is similar to how the `Viewport` component used to embed the id in these handlers (though you have just removed those in previous patches here).
> 
> What do you think?

I changed my approach, which I think will still be satisfactory to the eventual multi-viewport world. I imagine the DeviceSelector would have to be refactor again in multi-viewport world and we would once again provide a viewportId to it like in the past. So, I simply just passed "viewportId: 0" when we initialize the DeviceSelector.
Attachment #8984804 - Attachment is obsolete: true
Attachment #8984852 - Attachment is obsolete: true
Attachment #8984857 - Attachment is obsolete: true
Attachment #8984859 - Attachment is obsolete: true
Attachment #8984878 - Attachment is obsolete: true
Comment on attachment 8992370 [details]
Bug 1467572 - Part 5: Implement the new style for the global toolbar.

Sorry don't review part 5 yet. Just pushing a wip patch.
Attachment #8992370 - Flags: review?(jryans)
Attachment #8992370 - Flags: review?(jryans) → feedback?(jryans)
Attachment #8992370 - Flags: feedback?(jryans)
Comment on attachment 8996608 [details]
Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar.

Carrying over the r+
Attachment #8996608 - Flags: review?(jryans) → review+
Comment on attachment 8996609 [details]
Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar.

Carrying over the r+
Attachment #8996609 - Flags: review?(jryans) → review+
Comment on attachment 8996610 [details]
Bug 1467572 - Part 8: Add proper separators to the global toolbar.

Carrying over the r+
Attachment #8996610 - Flags: review?(jryans) → review+
Comment on attachment 8992370 [details]
Bug 1467572 - Part 5: Implement the new style for the global toolbar.

https://reviewboard.mozilla.org/r/257226/#review267800

I assume you're in progress of updating the styles to match https://mozilla.invisionapp.com/share/2NK8A5CWVPG#/screens
There are a few things still missing, like the viewport background, "Reload when..." icon, and consistent menu styles. Perhaps you're aware of those and will follow up here or in another patch.

::: devtools/client/responsive.html/components/Toolbar.js:122
(Diff revision 3)
> +      )
>      );
>    }
>  }
>  
>  module.exports = GlobalToolbar;

Rename class name and export from `GlobalToolbar` to `Toolbar`

::: devtools/client/responsive.html/index.css:43
(Diff revision 3)
>  
>  html,
>  body,
>  #root {
>    height: 100%;
> -  margin: 0;
> +  overflow: none;

There is no `overflow: none`. Perhaps you meant `overflow: hidden`?. A bot will backout the patch when it finds non-standard CSS properties or values.

::: devtools/client/responsive.html/index.css:145
(Diff revision 3)
>  .devtools-toggle-menu .devtools-menu {
>    display: none;
>    flex-direction: column;
>    align-items: start;
>    position: absolute;
>    left: 0;

Change `left: 0` to `right: 0` to prevent "Reload when..." menu from overflowing the viewport and introducing a horizontal scrollbar.

::: devtools/client/responsive.html/index.css:240
(Diff revision 3)
> -#global-screenshot-button:disabled {
> +#screenshot-button:disabled {
>    filter: var(--theme-icon-checked-filter);
>    opacity: 1 !important;
>  }
>  
> -#global-network-throttling-selector {
> +#network-throttling-selector {

This selector no longer matches. Either restore `#global-network-throttling-selector` or update the ID in https://searchfox.org/mozilla-central/source/devtools/client/shared/components/throttling/NetworkThrottlingSelector.js#102

Not sure if that impacts anything else that's using the shared component.

::: devtools/client/responsive.html/index.css:249
(Diff revision 3)
>  }
>  
> -#global-device-pixel-ratio-selector {
> +#device-pixel-ratio-selector {
>    -moz-user-select: none;
>    color: var(--viewport-color);
>    height: 15px;

Removing `height: 15px` from `#device-pixel-ratio-selector` and `#network-throttling-selector` makes the hit target area larger to click with the mouse.

The shorter right border could be reproduced with a solid gradient background and `background-size: 1px 15px; background-repeat: no-repeat;`
Attachment #8992370 - Flags: review?(rcaliman) → review+
Comment on attachment 8992370 [details]
Bug 1467572 - Part 5: Implement the new style for the global toolbar.

https://reviewboard.mozilla.org/r/257226/#review267800

> Removing `height: 15px` from `#device-pixel-ratio-selector` and `#network-throttling-selector` makes the hit target area larger to click with the mouse.
> 
> The shorter right border could be reproduced with a solid gradient background and `background-size: 1px 15px; background-repeat: no-repeat;`

I am gonna push these changes into a later patch since we want to change these selectors.
Comment on attachment 8996608 [details]
Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar.

Carrying over the previous r+
Attachment #8996608 - Flags: review?(jryans) → review+
Comment on attachment 8996609 [details]
Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar.

Carrying over the previous r+
Attachment #8996609 - Flags: review?(jryans) → review+
Comment on attachment 8996610 [details]
Bug 1467572 - Part 8: Add proper separators to the global toolbar.

Carrying over the previous r+
Attachment #8996610 - Flags: review?(jryans) → review+
Comment on attachment 8996610 [details]
Bug 1467572 - Part 8: Add proper separators to the global toolbar.

https://reviewboard.mozilla.org/r/260712/#review268546
Comment on attachment 8996608 [details]
Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar.

https://reviewboard.mozilla.org/r/260708/#review268548
Comment on attachment 8996609 [details]
Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar.

https://reviewboard.mozilla.org/r/260710/#review268550
Attachment #8997956 - Attachment is obsolete: true
Attachment #8997956 - Flags: review?(odvarko)
Comment on attachment 8998075 [details]
Bug 1467572 - Part 13: Removes the custom toolbar button styling and fixes the sizing of the device close button in RDM.

https://reviewboard.mozilla.org/r/261602/#review268770
Attachment #8998075 - Flags: review?(rcaliman) → review+
Comment on attachment 8997954 [details]
Bug 1467572 - Part 9: Use native context menu instead of select elements in the reload condition menu of RDM.

https://reviewboard.mozilla.org/r/261592/#review268912

Looks good to me.

R+ assuming my inline comments are resolved.

Honza

::: devtools/client/netmonitor/src/components/Toolbar.js:59
(Diff revision 5)
>  const NO_THROTTLING_LABEL = new LocalizationHelper(
>    "devtools/client/locales/network-throttling.properties"
>    ).getStr("responsive.noThrottling");
>  
>  // Menu
> -loader.lazyRequireGetter(this, "showMenu", "devtools/client/netmonitor/src/utils/menu", true);
> +loader.lazyRequireGetter(this, "showMenu", "devtools/client/shared/components/menu/utils/menu", true);

I would prefer if the existing utils `menu.js` file is renamed to `utils.js` and located along side the other files in the `menu` dir.

So, the path would be:

loader.lazyRequireGetter(this, "showMenu", "devtools/client/shared/components/menu/utils", true);

It's ok for me if it's done in another bug (but please file it in such case).

::: devtools/client/responsive.html/index.css:205
(Diff revision 5)
>  #screenshot-button::before {
>    background-image: url("./images/screenshot.svg");
>  }
>  
> +#settings-button::before {
> +  background-image: url("chrome://devtools/skin/images/settings.svg");

Can't we use the existing `var(--tool-options-image)` -> chrome://devtools/skin/images/tool-options.svg ?
Attachment #8997954 - Flags: review?(odvarko) → review+
Comment on attachment 8997955 [details]
Bug 1467572 - Part 10: Reuse the network throttling menu in the network monitor in RDM.

https://reviewboard.mozilla.org/r/261594/#review268914

r+ Assumming my inline comment is resolved.

Honza

::: devtools/client/themes/toolbars.css:238
(Diff revision 5)
> +}
> +
> +/* Make sure spacing between text and icon is uniform */
> +#network-throttling-menu .title {
> +  width: 85%;
> +}

These two CSS rules (related to #network-throttling-menu ) could stay in `devtools/client/netmonitor/src/assets/styles/Toolbar.css` no?
Attachment #8997955 - Flags: review?(odvarko) → review+
Comment on attachment 8998067 [details]
Bug 1467572 - Part 11: Use native context menu instead of select elements in the DPR menu.

https://reviewboard.mozilla.org/r/261598/#review268916

Looks good to me!
R+

Honza
Attachment #8998067 - Flags: review?(odvarko) → review+
Comment on attachment 8998068 [details]
Bug 1467572 - Part 12: Use native context menu instead of select element in the Device selector.

https://reviewboard.mozilla.org/r/261600/#review268918

Looks good
R+
Honza
Attachment #8998068 - Flags: review?(odvarko) → review+
Comment on attachment 8998317 [details]
Bug 1467572 - Part 14: Set the minimum viewport dimension to 50px.

https://reviewboard.mozilla.org/r/261606/#review268948

::: devtools/client/responsive.html/components/ViewportDimension.js:60
(Diff revision 2)
>    /**
>     * Return true if the given value is a number and greater than MIN_VIEWPORT_DIMENSION
>     * and false otherwise.
>     */
>    isInputValid(value) {
> -    return /^\d{3,4}$/.test(value) && parseInt(value, 10) >= MIN_VIEWPORT_DIMENSION;
> +    return /^\d{2,4}$/.test(value) && parseInt(value, 10) >= MIN_VIEWPORT_DIMENSION;

If the goal is to check that value is indeed any nymber and larger than MIN_VIEWPROT_DIMENSIONS you could contrive it to:

```
return parseInt(value, 10) >= MIN_VIEWPORT_DIMENSION;
```

The comparison with `NaN` returned by `parseInt()` will always return false so that's a good enough guard for number input.

The Regex may guard against very large numbers, but it will still pass for some large 4-digit value like `9999`. If the goal is to constrain it to an upper limit, perhaps that needs to be explicitly defined.
Attachment #8998317 - Flags: review?(rcaliman) → review+
Comment on attachment 8997954 [details]
Bug 1467572 - Part 9: Use native context menu instead of select elements in the reload condition menu of RDM.

https://reviewboard.mozilla.org/r/261592/#review268980

::: devtools/client/responsive.html/index.css:205
(Diff revision 5)
>  #screenshot-button::before {
>    background-image: url("./images/screenshot.svg");
>  }
>  
> +#settings-button::before {
> +  background-image: url("chrome://devtools/skin/images/settings.svg");

I spoke with Victoria about the settings icons over Slack and it was decided that we should consider using the photon settings icon to make it fit with all the other icons we have in the RDM toolbar.
Comment on attachment 8997955 [details]
Bug 1467572 - Part 10: Reuse the network throttling menu in the network monitor in RDM.

https://reviewboard.mozilla.org/r/261594/#review268982

::: devtools/client/themes/toolbars.css:238
(Diff revision 5)
> +}
> +
> +/* Make sure spacing between text and icon is uniform */
> +#network-throttling-menu .title {
> +  width: 85%;
> +}

I am reusing this style in the RDM Toolbar for the Network Throttling Menu as well so I needed to move the style to something that is loaded by RDM and Network Monitor.
Comment on attachment 8998317 [details]
Bug 1467572 - Part 14: Set the minimum viewport dimension to 50px.

https://reviewboard.mozilla.org/r/261606/#review268992

::: devtools/client/responsive.html/components/ViewportDimension.js:60
(Diff revision 2)
>    /**
>     * Return true if the given value is a number and greater than MIN_VIEWPORT_DIMENSION
>     * and false otherwise.
>     */
>    isInputValid(value) {
> -    return /^\d{3,4}$/.test(value) && parseInt(value, 10) >= MIN_VIEWPORT_DIMENSION;
> +    return /^\d{2,4}$/.test(value) && parseInt(value, 10) >= MIN_VIEWPORT_DIMENSION;

The regex is to guard against a number with a letter such as "209a" since parseInt("209a") will still parse this value into 209 whereas parseInt("a209") will return NaN. The upper range should be guarded from the explicit "size: 4" that is set on the <input>
Comment on attachment 8998741 [details]
Bug 1467572 - Part 16: Adjust RDM colors for the light and dark theme to match the designs.

https://reviewboard.mozilla.org/r/261648/#review269092
Attachment #8998741 - Flags: review?(rcaliman) → review+
Comment on attachment 8997955 [details]
Bug 1467572 - Part 10: Reuse the network throttling menu in the network monitor in RDM.

https://reviewboard.mozilla.org/r/261594/#review269102

::: devtools/client/netmonitor/src/assets/styles/variables.css
(Diff revision 6)
>    --primary-toolbar-height: 29px;
>  
>    /* Icons */
>    --play-icon-url: url("chrome://devtools/content/netmonitor/src/assets/icons/play.svg");
>    --pause-icon-url: url("chrome://devtools/skin/images/pause.svg");
> -  --drop-down-icon-url: url("chrome://devtools/content/netmonitor/src/assets/icons/drop-down.svg");

Heads-up that I copied this file to `client/themes/images/drop-down.svg` because I need it for the dropdowns in the font editor. I also declared ` --drop-down-icon-url` on the theme variables.css to point to the copied file.
Comment on attachment 8997955 [details]
Bug 1467572 - Part 10: Reuse the network throttling menu in the network monitor in RDM.

https://reviewboard.mozilla.org/r/261594/#review269104

::: devtools/client/netmonitor/src/assets/styles/variables.css
(Diff revision 6)
>    --primary-toolbar-height: 29px;
>  
>    /* Icons */
>    --play-icon-url: url("chrome://devtools/content/netmonitor/src/assets/icons/play.svg");
>    --pause-icon-url: url("chrome://devtools/skin/images/pause.svg");
> -  --drop-down-icon-url: url("chrome://devtools/content/netmonitor/src/assets/icons/drop-down.svg");

Ok. You might actually want to just move select-arrow.svg over to client/themes/images instead if you haven't landed your changes yet. You only have to adjust the width/height to be the same as drop-down.svg.

select-arrow.svg and drop-down.svg are essentially duplicated svg files. I chose to keep select-arrow since that was a bit more clear in terms of naming. The other aside is that drop-down is usually just one word "dropdown", which is also why I chose the select-arrow naming.
Comment on attachment 8999089 [details]
Bug 1467572 - Part 17: Implement left alignment of viewports.

https://reviewboard.mozilla.org/r/261668/#review269252

::: devtools/client/responsive.html/actions/moz.build:15
(Diff revision 1)
>      'index.js',
>      'location.js',
>      'reload-conditions.js',
>      'screenshot.js',
>      'touch-simulation.js',
> +    'ui.js',

Not a blocker, but perhaps something to consider for refactoring later: some of the actions and reducers are contextually relevant together in order to be joined in a single file. For example, both ui.js and touch-emulation.js do one thing only (for now) and could be joined in a single store/actions/reducers file, maybe named something like settings.js

::: devtools/client/responsive.html/actions/ui.js:18
(Diff revision 1)
> +    type: ENABLE_LEFT_ALIGNMENT,
> +    enabled,
> +  };
> +}
> +
> +function toggleLeftAlignment() {

You can avoid the extra indirection and calling the store state by handling the boolean toggle in the reducer.

For example, 

```
function toggleLeftAlignment(enabled) {
  return {
    type: ENABLE_LEFT_ALIGNMENT,
    enabled,
  };
}
```

and in the reducer check if `enabled` is undefined, then read from the prev state before setting the new one:


```
[ENABLE_LEFT_ALIGNMENT](ui, { enabled }) {
    enabled = enabled !== undefined ? enabled : !ui.leftAlignmentEnabled;
    return Object.assign({}, ui, {
      leftAlignmentEnabled: enabled,
    });
  },
```

With this change, you no longer need the additional `enableLeftAlignment()` action as you can reuse `toggleLeftAlignment(enabled)` with or without an argument.

The constant `ENABLE_LEFT_ALIGNMENT` is probably best renamed to `TOGGLE_LEFT_ALIGNMENT` but I won't block review on that.

::: devtools/client/responsive.html/components/SettingsMenu.js:88
(Diff revision 1)
>        })
>      );
>    }
>  }
>  
> -module.exports = SettingsMenu;
> +const mapStateToProps = state => {

This is cool! :)

::: devtools/client/responsive.html/reducers/ui.js:11
(Diff revision 1)
> +
> +const {
> +  ENABLE_LEFT_ALIGNMENT,
> +} = require("../actions/index");
> +
> +function UI() {

What's the reason for wrapping initial state in a function here instead of reusing the pattern with the plain object used in other reducers in this folder?
Attachment #8999089 - Flags: review?(rcaliman) → review+
Comment on attachment 8999089 [details]
Bug 1467572 - Part 17: Implement left alignment of viewports.

https://reviewboard.mozilla.org/r/261668/#review269252

> What's the reason for wrapping initial state in a function here instead of reusing the pattern with the plain object used in other reducers in this folder?

I was actually looking at the other panels like webconsole, application, and network to see what they were doing. Initially, my thinking was to follow what I did in https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/reducers/highlighter-settings.js#19 to avoid caching of the values when the toolbox is closed and reopened, but it seems like you previous suggestion also does the trick.
Comment on attachment 8999397 [details]
Bug 1467572 - Part 18: Show an onboarding tooltip for the setting menu button in RDM.

https://reviewboard.mozilla.org/r/261670/#review269320

Thanks for the patch Gabriel!

The new tooltip code shares a lot of similarities with the 3pane tooltip. 
Can you explain why we are duplicating or try to mutualize some of this code (followup is fine by me).

Would like to get an answer to this + r? to flod before r+'ing on my side.

::: devtools/client/locales/en-US/responsive.properties:136
(Diff revision 1)
> +# LOCALIZATION NOTE (responsive.settingOnboarding.content,
> +# responsive.settingOnboarding.learnMoreLink): This is the content shown in the setting
> +# onboarding tooltip that is displayed below the settings menu button in Responsive
> +# Design Mode. %S in the content will be replaced by a link at run time with the
> +# learnMoreLink string.
> +responsive.settingOnboarding.content=New: Change to left-alignment or edit reload behavior here. %S
> +responsive.settingOnboarding.learnMoreLink=Learn more

Please ping :flod for l10n review.

::: devtools/client/preferences/devtools-client.js:323
(Diff revision 1)
>  pref("devtools.responsive.reloadConditions.userAgent", false);
>  // Whether to show the notification about reloading to apply emulation
>  pref("devtools.responsive.reloadNotification.enabled", true);
> +// Whether to show the settings onboarding tooltip only in release or beta builds.
> +#if defined(RELEASE_OR_BETA)
> +pref("devtools.responsive.show-setting-tooltip", true);

Is this something we should force to false on test suites? (thinking about beta simulations etc....)

::: devtools/client/responsive.html/setting-onboarding-tooltip.js:17
(Diff revision 1)
> +const L10N = new LocalizationHelper("devtools/client/locales/responsive.properties");
> +
> +const SHOW_SETTING_TOOLTIP_PREF = "devtools.responsive.show-setting-tooltip";
> +
> +const CONTAINER_WIDTH = 300;
> +// TODO: Get a learn more link from mbalfanz before landing.

Not sure if I should call this out or if you are already tracking it. In doubt logging an issue here.

::: devtools/client/responsive.html/setting-onboarding-tooltip.js:24
(Diff revision 1)
> +
> +/**
> + * Setting onboarding tooltip that is shown on the setting menu button in the RDM toolbar
> + * when the pref is on.
> + */
> +class SettingOnboardingTooltip {

How much of this code is shared with the three pane tooltip? Why are we not trying to mutualize some code here? If this is because these tooltips are objectively shortlived, do we have bugs to remove the code?
Attachment #8999397 - Flags: review?(jdescottes)
It seems like you're getting rid of selects in the UI, so responsive-ua.css can be removed. ( https://searchfox.org/mozilla-central/search?q=responsive-ua.css&case=true&regexp=false&path= )
Flags: qe-verify+
Comment on attachment 8999397 [details]
Bug 1467572 - Part 18: Show an onboarding tooltip for the setting menu button in RDM.

https://reviewboard.mozilla.org/r/261670/#review269498

::: devtools/client/locales/en-US/responsive.properties:136
(Diff revision 1)
> +# LOCALIZATION NOTE (responsive.settingOnboarding.content,
> +# responsive.settingOnboarding.learnMoreLink): This is the content shown in the setting
> +# onboarding tooltip that is displayed below the settings menu button in Responsive
> +# Design Mode. %S in the content will be replaced by a link at run time with the
> +# learnMoreLink string.
> +responsive.settingOnboarding.content=New: Change to left-alignment or edit reload behavior here. %S
> +responsive.settingOnboarding.learnMoreLink=Learn more

Learn more string was removed.

::: devtools/client/preferences/devtools-client.js:323
(Diff revision 1)
>  pref("devtools.responsive.reloadConditions.userAgent", false);
>  // Whether to show the notification about reloading to apply emulation
>  pref("devtools.responsive.reloadNotification.enabled", true);
> +// Whether to show the settings onboarding tooltip only in release or beta builds.
> +#if defined(RELEASE_OR_BETA)
> +pref("devtools.responsive.show-setting-tooltip", true);

Fixed.

::: devtools/client/responsive.html/setting-onboarding-tooltip.js:17
(Diff revision 1)
> +const L10N = new LocalizationHelper("devtools/client/locales/responsive.properties");
> +
> +const SHOW_SETTING_TOOLTIP_PREF = "devtools.responsive.show-setting-tooltip";
> +
> +const CONTAINER_WIDTH = 300;
> +// TODO: Get a learn more link from mbalfanz before landing.

We're removing the learn morel links since there isn't enough content to make this worthwhile.

::: devtools/client/responsive.html/setting-onboarding-tooltip.js:24
(Diff revision 1)
> +
> +/**
> + * Setting onboarding tooltip that is shown on the setting menu button in the RDM toolbar
> + * when the pref is on.
> + */
> +class SettingOnboardingTooltip {

I do agree we should try to mutualize the code between the SettingsOnboardingTooltip and ThreePaneOnboardingTooltip, and it would've made sense still if the learn more link haven't been taken out. I think in the future new tooltips should simply be created from a common tooltip widget.
Comment on attachment 8999397 [details]
Bug 1467572 - Part 18: Show an onboarding tooltip for the setting menu button in RDM.

https://reviewboard.mozilla.org/r/261670/#review269510
Attachment #8999397 - Flags: review?(jdescottes) → review+
Comment on attachment 8999397 [details]
Bug 1467572 - Part 18: Show an onboarding tooltip for the setting menu button in RDM.

Francesco, can you have a quick look at this new string?
Attachment #8999397 - Flags: review?(francesco.lodolo)
Comment on attachment 8998727 [details]
Bug 1467572 - Part 15: Fix unit tests for RDM to work with the new RDM design.

https://reviewboard.mozilla.org/r/261646/#review269514
Attachment #8998727 - Flags: review?(odvarko) → review+
Comment on attachment 8999397 [details]
Bug 1467572 - Part 18: Show an onboarding tooltip for the setting menu button in RDM.

https://reviewboard.mozilla.org/r/261670/#review269516

I don't see any issue
Attachment #8999397 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 9000102 [details]
Bug 1467572 - Part 19: Remove responsive-ua.css.

https://reviewboard.mozilla.org/r/261684/#review269520
Attachment #9000102 - Flags: review?(rcaliman) → review+
Attachment #8996608 - Flags: review?(jryans) → review+
Attachment #8996609 - Flags: review?(jryans) → review+
Attachment #8996610 - Flags: review?(jryans) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7793f8a2b14f
Part 1: Move the App component into the component folder. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaea1245d27
Part 2: Remove the viewport toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/6102b9ef805b
Part 3: Remove the viewport dimension. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/934fb24dad26
Part 4: Move the device selector into the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/c42448ca6e4a
Part 5: Implement the new style for the global toolbar. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83526778ccd
Part 6: Add the viewport dimension to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f19f84bb2ed
Part 7: Add rotate viewport button to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/5880a55d2ee8
Part 8: Add proper separators to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/636bf5ce96f7
Part 9: Use native context menu instead of select elements in the reload condition menu of RDM. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/37609995dc8f
Part 10: Reuse the network throttling menu in the network monitor in RDM. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/470b9ce4ba0f
Part 11: Use native context menu instead of select elements in the DPR menu. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb52c2c68ab
Part 12: Use native context menu instead of select element in the Device selector. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/36d459a24020
Part 13: Removes the custom toolbar button styling and fixes the sizing of the device close button in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6b95a6c506
Part 14: Set the minimum viewport dimension to 50px. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d85a6f8a2a
Part 15: Fix unit tests for RDM to work with the new RDM design. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/e295be61276e
Part 16: Adjust RDM colors for the light and dark theme to match the designs. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef6c1c42747
Part 17: Implement left alignment of viewports. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/62fcbe25665c
Part 18: Show an onboarding tooltip for the setting menu button in RDM. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/39123899178c
Part 19: Remove responsive-ua.css. r=rcaliman
Backed out for devtools failures on browser_net_telemetry_throttle_changed.js

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2c0b075c0ec9bb9277835fbe510f81a3d0d2f9 

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=39123899178c01f6863d1949e315b6093dc1837f&filter-searchStr=devtools&group_state=expanded&selectedJob=194196401

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194196401&repo=mozilla-inbound&lineNumber=2953

[task 2018-08-15T23:22:34.323Z] 23:22:34     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js | No events have been logged for the main process - 
[task 2018-08-15T23:22:34.323Z] 23:22:34     INFO - Buffered messages finished
[task 2018-08-15T23:22:34.324Z] 23:22:34     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js:28 - TypeError: document.querySelector(...) is null
[task 2018-08-15T23:22:34.324Z] 23:22:34     INFO - Stack trace:
[task 2018-08-15T23:22:34.324Z] 23:22:34     INFO - @chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js:28:3
[task 2018-08-15T23:22:34.324Z] 23:22:34     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1106:34
[task 2018-08-15T23:22:34.325Z] 23:22:34     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1097:16
[task 2018-08-15T23:22:34.325Z] 23:22:34     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:999:9
[task 2018-08-15T23:22:34.326Z] 23:22:34     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2018-08-15T23:22:34.326Z] 23:22:34     INFO - Leaving test bound 
[task 2018-08-15T23:22:34.692Z] 23:22:34     INFO - Removing tab.
[task 2018-08-15T23:22:34.694Z] 23:22:34     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-08-15T23:22:34.730Z] 23:22:34     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-08-15T23:22:34.738Z] 23:22:34     INFO - Tab removed and finished closing
[task 2018-08-15T23:22:34.739Z] 23:22:34     INFO - finish() was called, cleaning up...
[task 2018-08-15T23:22:34.796Z] 23:22:34     INFO - GECKO(2195) | MEMORY STAT | vsize 20975803MB | residentFast 3027MB
[task 2018-08-15T23:22:34.796Z] 23:22:34     INFO - TEST-OK | devtools/client/netmonitor/test/browser_net_telemetry_throttle_changed.js | took 3585ms
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcedacae2157
Part 1: Move the App component into the component folder. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/303f375007db
Part 2: Remove the viewport toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/02aeb2593935
Part 3: Remove the viewport dimension. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/3427989ad89e
Part 4: Move the device selector into the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae96e14900f
Part 5: Implement the new style for the global toolbar. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1db12b0ab9
Part 6: Add the viewport dimension to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/c208d5b5b746
Part 7: Add rotate viewport button to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2ea377f944
Part 8: Add proper separators to the global toolbar. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d81c2f9098
Part 9: Use native context menu instead of select elements in the reload condition menu of RDM. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/b55503cf4ad4
Part 10: Reuse the network throttling menu in the network monitor in RDM. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed506e2100a
Part 11: Use native context menu instead of select elements in the DPR menu. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/06c72f105c50
Part 12: Use native context menu instead of select element in the Device selector. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb675aaabac
Part 13: Removes the custom toolbar button styling and fixes the sizing of the device close button in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f34ca109aac
Part 14: Set the minimum viewport dimension to 50px. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e8061780fd
Part 15: Fix unit tests for RDM to work with the new RDM design. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/3199491bd055
Part 16: Adjust RDM colors for the light and dark theme to match the designs. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/599eb93053e0
Part 17: Implement left alignment of viewports. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6965e5700a
Part 18: Show an onboarding tooltip for the setting menu button in RDM. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/7080672c7b8e
Part 19: Remove responsive-ua.css. r=rcaliman
Comment on attachment 8998068 [details]
Bug 1467572 - Part 12: Use native context menu instead of select element in the Device selector.

https://reviewboard.mozilla.org/r/261600/#review269570

::: devtools/client/locales/en-US/responsive.properties:16
(Diff revision 10)
>  # A good criteria is the language in which you'd find the best
>  # documentation on web development on the web.
>  
> -# LOCALIZATION NOTE (responsive.editDeviceList): option displayed in the device
> -# selector
> -responsive.editDeviceList=Edit list…
> +# LOCALIZATION NOTE (responsive.editDeviceList2): Context menu item displayed in the
> +# device selector.
> +responsive.editDeviceList2=Edit List…

For future reference: changing capitalization doesn't require a new string ID. Actually, it's better without a new ID, to not force every locale to retranslate.
Depends on: 1488566
Depends on: 1498486
Verified with 63.0b13. 
Implementation is as in mock-ups on Windows10, MacOS10.13, Ubuntu 16.04.

Logged bug 1498486 as a separate issue and several others regarding the RDM edit/device modal list that where before this item as well.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1516275
Regressions: 1592197
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: