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!

For reference, this is based on Victoria's designs: https://mozilla.invisionapp.com/share/2NK8A5CWVPG

Comment on attachment 8984800 [details] Bug 1467572 - Part 2: Remove the viewport toolbar. https://reviewboard.mozilla.org/r/250602/#review257064

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?

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?

(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.

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?

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

Comment on attachment 8984859 [details] Bug 1467572 - Part 8: Add proper separators to the global toolbar. https://reviewboard.mozilla.org/r/250658/#review257110

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.

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...?

(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...?

I was looking at https://mozilla.invisionapp.com/d/main#/console/12408235/260632515/preview

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.

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.

(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.

(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!!)

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.

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.

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.

Comment on attachment 8996608 [details] Bug 1467572 - Part 6: Add the viewport dimension to the global toolbar. Carrying over the r+

Comment on attachment 8996609 [details] Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar. Carrying over the r+

Comment on attachment 8996610 [details] Bug 1467572 - Part 8: Add proper separators to the global toolbar. Carrying over the r+

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;`

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+

Comment on attachment 8996609 [details] Bug 1467572 - Part 7: Add rotate viewport button to the global toolbar. Carrying over the previous r+

Comment on attachment 8996610 [details] Bug 1467572 - Part 8: Add proper separators to the global toolbar. Carrying over the previous r+

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

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

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 ?

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?

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

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

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.

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

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.