1266411 - The devices in the select box should be sorted alphabetically

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Comment 1

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch

Comment 2

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

Comment on attachment 8755878 [details] [diff] [review]
1266411.patch

Review of attachment 8755878 [details] [diff] [review]:
-----------------------------------------------------------------

Please update your commit message to describe what you changed, instead of restating the bug title.

Thanks for taking a look at this!

::: devtools/client/responsive.html/components/device-selector.js
@@ +56,5 @@
>      } = this.props;
>  
>      let options = [];
>      for (let type of devices.types) {
> +      devices[type].sort(function (a, b) {

This is close, but it will only sort devices within a single type.

My impression from Helen's UX notes (bug 1241713 comment 5) is that the goal is to sort the entire list of devices, regardless of type.

How about after these loops are done, you sort the `options` array instead?  I think that should give the desired effect.

We can double check with Helen by requesting a ui-review on the next patch.

Comment 3

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [1.0]

Comment 4

[Jaideep Bhoosreddy [:jbhoosreddy]]

I had misunderstood the bug earlier so I ended up doing something else. I've submitted a new patch for this.

Comment 5

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

Comment on attachment 8756119 [details] [diff] [review]
1266411.patch [1.0]

It looks like your new patch is an interdiff applied on top of the first patch.  Can you squash them together instead?

Also, since I did not give an r+ before, I still need to review new versions as well.  Once you've gotten an r+ on a past version, it can be reasonable to carry it forward as you fix review nits, etc.

So, I'll clear the flags here.  Once you've fixed the patch, set review? to me and ui-review? to Helen.

Comment 6

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [2.0]

Comment 7

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

Comment on attachment 8756163 [details] [diff] [review]
1266411.patch [2.0]

Review of attachment 8756163 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks for working on it.

Do you have access to the try server yet[1]?  If not, you should request it and have one of us vouch for you.

Once there is a green try run for this change, you can add "checkin-needed" to the keywords field on this bug and a sheriff will land the change for you.

[1]: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/25566a14a7ac

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25566a14a7ac

Comment 10

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 48.0a1 (2016-04-03) and on Windows 10 x64.
In the dropdown list, the devices are arranged alphabetically, but in the 'Edit list' -> Televisions, the devices are arranged in in the quality order (http://imgur.com/a79xiuF).
I'm not sure if this is an issue, but it is an inconsistency.
The tests were performed on Firefox 49.0a1 (2016-05-27) and on Windows 10 x64, Ubuntu 14.04 x64, Mac OS X 10.10.5.

Ryan, should I reopen this issue, or it's an expected result?

Comment 11

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

Huh, it looks like the device modal is just displaying the devices in the order they appear in the DB.  It just happens that most of the entries are already sorted!

Thanks for spotting this Mihai!  Jaideep, let's make another small tweak to sort devices within a category for the device modal.

Comment 12

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [3.0]

Comment 13

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

Since you're adding totally new functionality here, I think a new review would be appropriate here.  For example, why is this change in the device selector?  I can look more next week.

Also, this should be a separate patch, since the original one already landed (it should not contain the things already landed).

Comment 14

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [4.0]

Thanks for all the pointers. I'm uploading the patch with the changes that you said.

Comment 15

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

Comment on attachment 8757668 [details] [diff] [review]
1266411.patch [4.0]

Review of attachment 8757668 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/components/device-selector.js
@@ +56,5 @@
>      } = this.props;
>  
>      let options = [];
>      for (let type of devices.types) {
> +      devices[type].sort((a, b) => {

We don't want to sort the devices in place because that would modify the state of the Redux store at render time.  The render() functions should take in state from props and display it, not modify it.

Additionally, we want to sort the device as displayed in the _device modal_, so I would expect to see a change in device-modal.js, not here.

For example, one way might be to sort the result of `devices[type].map` in that file.  Or you could clone `device[type]` and sort that before it is mapped to elements.

Comment 16

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [5.0]

Comment 17

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

Comment on attachment 8758380 [details] [diff] [review]
1266411.patch [5.0]

Review of attachment 8758380 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/components/device-modal.js
@@ +27,5 @@
>  
> +  componentWillMount() {
> +    let { devices } = this.props;
> +    for (let type of devices.types) {
> +      devices[type].sort((a, b) => {

We don't want to mutate data that comes from `props`.  See more from React[1] about this. `sort` runs in place, so you need to run it on a copy of the array.

Since this is a rendering concern, it seems like this work should happen as part of `render`, not `componentWillMount`.  If it was expensive work, we could think of something more clever here, but there are only a few items here, so it seems fine to do the sort during `render`.  This makes it easier to copy the array and preserve the immutability of props as well.

[1]: http://facebook.github.io/react/docs/jsx-spread.html#mutating-props-is-bad

Comment 18

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [6.0]

Comment 19

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [6.0]

Comment 20

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

Comment on attachment 8758867 [details] [diff] [review]
1266411.patch [6.0]

Review of attachment 8758867 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a small tweak to make.

It should be ready to land (no need for another review) after you make that change.  Normally we'd run on try, but perhaps the sheriffs will land this one as well without it (they used to always require a try run).

::: devtools/client/responsive.html/components/device-modal.js
@@ +85,5 @@
>  
> +    const sortedDevices = {};
> +    for (let type of devices.types) {
> +      sortedDevices[type] = Object.assign([], devices[type])
> +        .sort((a, b) => {

Since it's a short function, you probably don't need a full block here:

  .sort((a, b) => a.name.localeCompare(b.name));

(`return` is implied for => functions without brackets.)

Comment 21

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1266411.patch [7.0]

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e3d9990ca6ec
Alphabetically sort devices in each type. r=jryans

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3d9990ca6ec

Comment 24

[Mihai Boldan, Desktop QA [:mboldan]]

The devices in the select box are sorted alphabetically.
The tests were performed on Firefox 50.0a1 (2016-06-07) and on Mac OS X 10.10.5, WIndows 10 x64 and on Ubuntu 14.04 x64.
I'm marking this issue Verified Fixed.