1471284 - Adjust appearance of throttling menu to match other controls

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The throttling menu in the Network Monitor looks a bit out of place with the other controls since it was borrowed from RDM's design.

For example:

* The menu background color changes depending on whether you hover the menu
* Custom HTML menu instead of native menu like HAR next to it

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@jryans: Thanks for reporting this!


Some comments for anyone interested in fixing this bug:

1) The Network panel toolbar is rendering the throttling button here:
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/client/netmonitor/src/components/Toolbar.js#284

2) The button is implemented as `NetworkThrottlingSelector` component that is located in this directory:
https://searchfox.org/mozilla-central/source/devtools/client/shared/components/throttling/

The directory contains other modules related to throttling and shared between RDM and Network panel.

3) The work on this bug will mostly be related to two things:

A) CSS: Network panel already needs some specific CSS styles for the button here:
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/client/netmonitor/src/assets/styles/Toolbar.css#123

So, any new styles should be appended at the same place (some styles might be actually removed).

B) DropDown: The button (NetworkThrottlingSelector) should use native context menu just like the HAR button (located next to it).

Simple solution can be using the same approach as the HAR button.
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/client/netmonitor/src/components/Toolbar.js#298

... and generate the list of items from existing `throttlingProfiles` similarly to how `NetworkThrottlingSelector` generates its custom HTML drop down.

https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/client/shared/components/throttling/NetworkThrottlingSelector.js#90

This way we stop sharing the `NetworkThrottlingSelector` component, but this feels ok since there are different UI requirements. And of course, we continue sharing the rest of the 'throttling' modules.

Honza

Comment 2

[Hemanth Kumar Veeranki [:harry7]]

(In reply to Jan Honza Odvarko [:Honza] from comment #1)
I am interested in taking up with this bug. I have few doubts after looking at your solution

> DropDown: The button (NetworkThrottlingSelector) should use native context menu just like the HAR button (located next to it).

By looking at this, I thought that the changes are supposed to be done in the NetworkThrottlingSelector 

> This way we stop sharing the `NetworkThrottlingSelector` component, but this
> feels ok since there are different UI requirements. And of course, we
> continue sharing the rest of the 'throttling' modules.

But here you said that we will stop sharing `NetworkThrottlingSelector`. So where should the changes be done? or did I misinterpret your lines?

Thanks,
Hemanth

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #2)
> (In reply to Jan Honza Odvarko [:Honza] from comment #1)
> I am interested in taking up with this bug.
Excellent!

> I have few doubts after looking
> at your solution
> 
> > DropDown: The button (NetworkThrottlingSelector) should use native context menu just like the HAR button (located next to it).
> 
> By looking at this, I thought that the changes are supposed to be done in
> the NetworkThrottlingSelector 
No, I would keep the `NetworkThrottlingSelector` as is and stop using it entirely.
See more details below. 

> > This way we stop sharing the `NetworkThrottlingSelector` component, but this
> > feels ok since there are different UI requirements. And of course, we
> > continue sharing the rest of the 'throttling' modules.
> 
> But here you said that we will stop sharing `NetworkThrottlingSelector`. So
> where should the changes be done? or did I misinterpret your lines?

I think that we can implement the Network throttling button similarly to how the HAR button is implemented and stop using `NetworkThrottlingSelector` entirely.

So, the `renderThrottlingSelector` would stop using the component and render simple `button` instead:
https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/client/netmonitor/src/components/Toolbar.js#284

Something like as follows:

return button({
  id: "devtools-throttling-button",
  title: TOOLBAR_THROTTLING_BUTTON,
  className: "devtools-button devtools-throttling-button",
  onClick: evt => {
    this.showThrottlingMenu(evt.target);
  },
});

Next, we need to implement the `showThrottlingMenu` method, which would be similar to how `showHarMenu` is done. I.e. we would calculate `menuItems` array from existing throttling profiles - similarly how the `NetworkThrottlingSelector` is doing it, but producing list of menu items instead of list of `options`.

Something like:

const menuItems = throttlingProfiles.map(profile => {
  return {
    id: "request-list-context-throttling-" + profile.id,
    label: L10N.getStr("netmonitor.context." + profile.id ),
    accesskey: L10N.getStr("netmonitor.context." + profile.id + ".accesskey"),
    click: () => this.onSelectChange(profile.id),
  });
}),

showMenu(menuItems, { button: menuButton });

The code above only generates throttling items, but there should also be "no throttling" and separator menu items added to the beginning of the menuItems list.

Does that make more sense?

Should I assign the bug to you?


Honza

Comment 4

[Hemanth Kumar Veeranki [:harry7]]

> Should I assign the bug to you?

Thanks for the reply Honza. Yeah please assign the bug to me. 

> const menuItems = throttlingProfiles.map(profile => {
>   return {
>     id: "request-list-context-throttling-" + profile.id,
>     label: L10N.getStr("netmonitor.context." + profile.id ),
>     accesskey: L10N.getStr("netmonitor.context." + profile.id +
> ".accesskey"),
>     click: () => this.onSelectChange(profile.id),
>   });
> }),

I started working on this bug and was able to generate all the functionality, I generated the menu items similar to the way you have mentioned. There was no accesskey and label information available in the netmonitor locale files. So I did not add them to the menu item and similarly I looked at how NetworkThrottlingSelector is handling click and replicated the functionality.

However I am stuck at the part where the button should display the selected throttling profile. I am unable to figure out how to do that. I would be glad if someone could help me out with that. Also should I submit a patch with the work I did till now?

Thanks,
Hemanth

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #4)
> I started working on this bug and was able to generate all the
> functionality, I generated the menu items similar to the way you have
> mentioned. There was no accesskey and label information available in the
> netmonitor locale files. So I did not add them to the menu item and
I thin that we don't need the access key for every throttling profile.
Just skip this.

> similarly I looked at how NetworkThrottlingSelector is handling click and
> replicated the functionality.
> 
> However I am stuck at the part where the button should display the selected
> throttling profile. I am unable to figure out how to do that. I would be

So, the title of the button should be generated according to the current profile.
I think thinking about something like as follows:

return button({
  id: "devtools-throttling-button",
  title: currentProfile.id,
  className: "devtools-button devtools-throttling-button",
  onClick: evt => {
    this.showThrottlingMenu(evt.target);
  },
});

If there is no profile selected the button needs to say "No throttling",
which is an existing string we can use:

L10N.getStr("responsive.noThrottling")

> glad if someone could help me out with that. Also should I submit a patch
> with the work I did till now?
Yes, please attach the patch here even if it's work in progress.

Thanks,
Honza

Comment 6

[Hemanth Kumar Veeranki [:harry7]]

Attachment: Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review commit: https://reviewboard.mozilla.org/r/257482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257482/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264378


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/Toolbar.js:300
(Diff revision 1)
> +        this.showThrottlingSelector(evt.target);
> +      },
> +    });
> +  }
> +
> +  showThrottlingSelector(menuButton){

Error: Missing space before opening brace. [eslint: space-before-blocks]

::: devtools/client/netmonitor/src/components/Toolbar.js:302
(Diff revision 1)
> +    });
> +  }
> +
> +  showThrottlingSelector(menuButton){
> +    const {
>        networkThrottling,

Error: 'networkThrottling' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/Toolbar.js:307
(Diff revision 1)
>        networkThrottling,
>        onChangeNetworkThrottling,
> +    } = this.props;
> +
> +    const menuItems = throttlingProfiles.map(profile => {
> +      return{

Error: Expected space(s) after "return". [eslint: keyword-spacing]

::: devtools/client/netmonitor/src/components/Toolbar.js:308
(Diff revision 1)
>        onChangeNetworkThrottling,
> +    } = this.props;
> +
> +    const menuItems = throttlingProfiles.map(profile => {
> +      return{
> +        id: "request-list-context-throttling-" + profile.id.toLocaleLowerCase().split(' ').join('-'),

Error: Line 308 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/netmonitor/src/components/Toolbar.js:308
(Diff revision 1)
>        onChangeNetworkThrottling,
> +    } = this.props;
> +
> +    const menuItems = throttlingProfiles.map(profile => {
> +      return{
> +        id: "request-list-context-throttling-" + profile.id.toLocaleLowerCase().split(' ').join('-'),

Error: Strings must use doublequote. [eslint: quotes]

::: devtools/client/netmonitor/src/components/Toolbar.js:308
(Diff revision 1)
>        onChangeNetworkThrottling,
> +    } = this.props;
> +
> +    const menuItems = throttlingProfiles.map(profile => {
> +      return{
> +        id: "request-list-context-throttling-" + profile.id.toLocaleLowerCase().split(' ').join('-'),

Error: Strings must use doublequote. [eslint: quotes]

Comment 8

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/1-2/

Comment 9

[Hemanth Kumar Veeranki [:harry7]]

> Yes, please attach the patch here even if it's work in progress.
I pushed my changes to the review board.

PS: Sorry for the lint errors, I fixed them and pushed the changes.

Thanks,
Hemanth

Comment 10

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/2-3/

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264578

Thanks for the patch!

Please see my inline comments.


Also, there are some visual discrepancies we need to fix:
- The button is too wide when 'No throttling' label is displayed. This takes unneccessary (valuable) space on the toolbar. The width should be the same as before. 
- The double arrow (indicating drop-down) should be v-centered the same way as for the HAR buton. It's possible that we need to use the same CSS style technique for the HAR button as well to get the same visual appearance
- The background gray color & size used when hovering the button is different from the HAR button. This should be unified. It wasn't perfect beore, but we can take the opportunity to fix it now.
- When selecting the logest label `Regular 4G / LTE` the text overlaps the icon. It should't happen the text should by cut (overflow hidden on the right side)

Some screenshots coming...

Honza

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:131
(Diff revision 3)
>    background-color: transparent;
>  }
>  
>  #global-network-throttling-selector {
>    width: 92px;
> -  padding-right: 12px;
> +  padding-right: 120px;

This padding makes the button too wide in the default 'No throttling' case (which most of the users will use). The button should have the same width as before.

::: devtools/client/netmonitor/src/components/Toolbar.js:21
(Diff revision 3)
>    getDisplayedRequests,
>    getRecordingState,
>    getTypeFilteredRequests,
>  } = require("../selectors/index");
>  const { autocompleteProvider } = require("../utils/filter-autocomplete-provider");
> +const { LocalizationHelper } = require("devtools/shared/l10n");

You don't need the LocalizationHelper here. Just use `L10N` (it's shared instance of LocalizationHelper)

::: devtools/client/netmonitor/src/components/Toolbar.js:296
(Diff revision 3)
> +    if (networkThrottling.enabled) {
> +      selectedProfile = networkThrottling.profile;
> +    } else {
> +      selectedProfile = new LocalizationHelper(
> +        "devtools/client/locales/network-throttling.properties"
> +        ).getStr("responsive.noThrottling");

You should use `L10N` like as follows:

L10N.getStr("responsive.noThrottling");

Also you can load the string just once at the top of the file. See the 'Localization' section where other strings are loaded.

::: devtools/client/netmonitor/src/components/Toolbar.js:317
(Diff revision 3)
> +    } = this.props;
> +
> +    const menuItems = throttlingProfiles.map(profile => {
> +      return {
> +        id: "request-list-context-throttling-"
> +        + profile.id.toLocaleLowerCase().split(" ").join("-"),

I think that we actually don't have to specify any id for the menu item at this moment. So, it can be removed.

::: devtools/client/netmonitor/src/components/Toolbar.js:329
(Diff revision 3)
> +
> +    menuItems.unshift({
> +      id: "request-list-context-throttling-disabled",
> +      label: new LocalizationHelper(
> +        "devtools/client/locales/network-throttling.properties"
> +      ).getStr("responsive.noThrottling"),

You should use a constant representing the string defined at the top of the file (as I mentioned in previous comment)

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: throttling-button.jpg

Comment 13

[Hemanth Kumar Veeranki [:harry7]]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)

> You don't need the LocalizationHelper here. Just use `L10N` (it's shared
> instance of LocalizationHelper)


The L10N being used in this file reads from a different file than "devtools/client/locales/network-throttling.properties". https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/l10n.js#9
This was the reason I used a new variable.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #13)
> The L10N being used in this file reads from a different file than
> "devtools/client/locales/network-throttling.properties".
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> utils/l10n.js#9
> This was the reason I used a new variable.

Ah, I see. So, you should create a new `L10N` at the top of the file (so, it executes just once), load strings at the top too (again, executed once) and consequently use string constants in the code.

Honza

Comment 15

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264578

> This padding makes the button too wide in the default 'No throttling' case (which most of the users will use). The button should have the same width as before.

Removed the padding and added a new approach now the button has same width as before.

> You should use `L10N` like as follows:
> 
> L10N.getStr("responsive.noThrottling");
> 
> Also you can load the string just once at the top of the file. See the 'Localization' section where other strings are loaded.

Changed the code to load once at the top of the file.

> I think that we actually don't have to specify any id for the menu item at this moment. So, it can be removed.

Removed the id from menu items.

> You should use a constant representing the string defined at the top of the file (as I mentioned in previous comment)

Changed the code accordingly.

Comment 16

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/3-4/

Comment 17

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264578

> You don't need the LocalizationHelper here. Just use `L10N` (it's shared instance of LocalizationHelper)

Can't use the same helper because we need to load a different `locales` file

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264690

Nice!

More comments:
- Please apply the same style on the HAR button
- The text should be at the same base line as e.g. the "Disable Cache" button (see the upcoming screenshot)
- Could we try to move the double-arrow icon 1px down (to v-center it). Of course, both icons (even for the HAR button should be at the same line)

Thanks for helping with this!
Honza

::: devtools/client/netmonitor/src/components/Toolbar.js:310
(Diff revision 4)
> +      },
> +    },
> +    dom.span(
> +      {
> +        className: "value-holder",
> +      },

Please format this piece as follows:

dom.span({className: "value-holder"},
  selectedProfile
));

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: net-toolbar.jpg

Comment 20

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264690

- Added similar css and button for HAR button
- Added center alignment for icons in css
- Adjusted the `span` inside the button so that, text will be aligned with others

> Please format this piece as follows:
> 
> dom.span({className: "value-holder"},
>   selectedProfile
> ));

Made changes and pushed them

Comment 21

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/4-5/

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264930

Thanks for the update, this is going in the right direction!

Please see my inline comments (mostly minor)

Comments realated to positioning:

- 'No Throttling' is using font-size 12px, 'HAR' is using 11px. They should both use 12px
- The labels are not on the same base line
- Gaps between labels and icons are different (they should be the same)

A screenshot describing in details what we want is coming...

Thanks,
Honza

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:84
(Diff revision 5)
> -.network-monitor .devtools-button.devtools-har-button:hover {
> +/* HAR button */
> -  background: var(--toolbarbutton-hover-background);
> -  border-color: var(--toolbarbutton-hover-border-color);
> -}
>  
> -/* HAR button has label and icon, so make sure they don't overlap */
> +#devtools-har-button{

nit: space between the selector and {

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:99
(Diff revision 5)
> +
> +#devtools-har-button:not(:hover) {
> +  background-color: transparent;
> +}
> +
> +#devtools-har-button .value-holder{

nit: space between the selector and {

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:148
(Diff revision 5)
>    padding-left: 0;
> +  overflow: hidden;
>    -moz-context-properties: fill;
>  }
>  
> -#global-network-throttling-selector > option.divider {
> +.value-holder {

Please use more specific selector

.devtools-button .value-holder

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: har-and-throttle-buttons.jpg

Detailed description of labels & icons positioning.

Honza

Comment 24

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review264930

- Updated the code with changes to the minor points
- Removed specific font-sizes for either of them, so they will use default font size 
- Adjusted width for `HAR` button so that they will have similar gaps
- Alligned labels to same base line

Comment 25

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/5-6/

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265052

This looks great! Tested on Win10 & OSX

The background highlighted when mouse is hoverin over has different height. I think that the only problem is the 25px height specified for HAR button.

Last thing, I am wondering if we could generalize the CSS somehow. I.e. introduce a new class like `devtools-dropdown-button` that would have all the common styles and consequently `#devtools-har-button` and `obal-network-throttling-selector` would define only spefic things. This way we could easily introduce new drop down button later and share the styles.

`.devtools-button .value-holder` could then change to `.devtools-dropdown-button .value-holder` or perhaps even better to `.devtools-dropdown-button .title`, which sounds more accurate.

Honza

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:92
(Diff revision 6)
>    background-position: right center;
> +  background-repeat: no-repeat;
>    fill: var(--theme-toolbar-photon-icon-color);
> -  height: 17px;
> +  -moz-context-properties: fill;
> +  margin-inline-start: 6px;
> +  height: 25px;

Why the height is set to 25? Can we remove that?

::: devtools/client/netmonitor/src/components/Toolbar.js:297
(Diff revision 6)
> -    return NetworkThrottlingSelector({
> +    let selectedProfile;
> +    if (networkThrottling.enabled) {
> +      selectedProfile = networkThrottling.profile;
> +    } else {
> +      selectedProfile = NO_THROTTLING_LABEL;
> +    }

nit: Can you please use conditional ternary operator instead.

Comment 27

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/6-7/

Comment 28

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265052

- Generalised most part of the css and changed the class name to `title` as suggested.
- Added the class `devtools-dropdown-button` to both har and throttling selctor buttons.

> Why the height is set to 25? Can we remove that?

`HAR` button initially had a height attribute, so I was updating it to match the current behaviour. Realised now that removing it doesn't effect its position. So removed it.

> nit: Can you please use conditional ternary operator instead.

Changed the code to use conditional operator.

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265120

Looks Awesome!

What about moving more css props into the common `devtools-drop-down` button?

background-repeat: no-repeat;
fill: var(--theme-toolbar-photon-icon-color);
-moz-context-properties: fill;
background-image: var(--drop-down-icon-url);
margin-inline-start: 6px;

All these are duped in `margin-inline-start: 6px;` and `#global-network-throttling-selector`

Honza

::: devtools/client/netmonitor/src/components/Toolbar.js:293
(Diff revision 7)
>        networkThrottling,
> -      onChangeNetworkThrottling,
>      } = this.props;
>  
> -    return NetworkThrottlingSelector({
> -      className: "devtools-button",
> +    const selectedProfile = networkThrottling.enabled ?
> +    networkThrottling.profile : NO_THROTTLING_LABEL;

nit: indentation

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91a0cd4f5138331773c431a8d831764c7cdb687e

Honza

Comment 31

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/7-8/

Comment 32

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265120

Thanks for pointing this out. I made changes accordingly and also indentation.

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265336

Very well done. Tested on Win10 (Retina, non-retina) as well as Mac, looks great!

Just two las minor comments.

Honza

::: devtools/client/netmonitor/src/assets/styles/Toolbar.css:119
(Diff revision 8)
> +}
> +
>  /* Make sure the HAR button label is vertically centered on Mac */
>  :root[platform="mac"] .devtools-button.devtools-har-button::before {
>    height: 14px;
>  }

Please remove this mac specific style, not needed anymore

::: devtools/client/netmonitor/src/components/Toolbar.js:293
(Diff revision 8)
>        networkThrottling,
> -      onChangeNetworkThrottling,
>      } = this.props;
>  
> -    return NetworkThrottlingSelector({
> -      className: "devtools-button",
> +    const selectedProfile = networkThrottling.enabled ?
> +     networkThrottling.profile : NO_THROTTLING_LABEL;

The indentation needs to be two spaces.

Comment 34

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/8-9/

Comment 35

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265336

Made changes as suggested
Hemanth

> Please remove this mac specific style, not needed anymore

Removed

> The indentation needs to be two spaces.

I see. I used online js beautifier to indent. I was not aware of the rules for such expressions

Comment 36

[Hemanth Kumar Veeranki [:harry7]]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257482/diff/9-10/

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8992608 [details]
Bug 1471284 - Adjust appearance of throttling menu to match other controls;

https://reviewboard.mozilla.org/r/257482/#review265356

Great job here!
Retested on Win10 and OSX

R+

Honza

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@cfogel: does it look good for you now?

Honza

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54e79326147a
Adjust appearance of throttling menu to match other controls; r=Honza

Comment 40

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/54e79326147a

Comment 41

[Cristian Fogel [:cfogel]]

(In reply to Jan Honza Odvarko [:Honza] PTO from 7/22 to 8/5 from comment #38)
> @cfogel: does it look good for you now?
> 
> Honza

Looks good!
Already managed to check with 63.0a1 (2018-07-22) on Win 10x64, macOS 10.13.4, Ubuntu 16.04LTS. Can confirm the new design is as intended.

I've noticed another small issue around this area: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1477679