Closed Bug 1266411 Opened 4 years ago Closed 3 years ago

The devices in the select box should be sorted alphabetically

Categories

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

defect

Tracking

(firefox49 fixed, firefox50 verified)

VERIFIED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed
firefox50 --- verified

People

(Reporter: gl, Assigned: jbhoosreddy)

References

Details

(Whiteboard: [multiviewport] [reserve-rdm])

Attachments

(1 file, 8 obsolete files)

No description provided.
Whiteboard: [multiviewport] [mvp-rdm]
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [mvp-rdm] [triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: mihai.boldan
Whiteboard: [multiviewport] [mvp-rdm] [triage] → [multiviewport] [mvp-rdm]
Priority: P2 → P3
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [reserve-rdm]
Assignee: nobody → jaideepb
Attached patch 1266411.patch (obsolete) — Splinter Review
Attachment #8755878 - Flags: review?(jryans)
Status: NEW → ASSIGNED
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.
Attachment #8755878 - Flags: review?(jryans) → review-
Attached patch 1266411.patch [1.0] (obsolete) — Splinter Review
Attachment #8755878 - Attachment is obsolete: true
Attachment #8756119 - Flags: ui-review?(hholmes)
Attachment #8756119 - Flags: review+
I had misunderstood the bug earlier so I ended up doing something else. I've submitted a new patch for this.
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.
Attachment #8756119 - Flags: ui-review?(hholmes)
Attachment #8756119 - Flags: review+
Attached patch 1266411.patch [2.0] (obsolete) — Splinter Review
Attachment #8756119 - Attachment is obsolete: true
Attachment #8756163 - Flags: ui-review?(hholmes)
Attachment #8756163 - Flags: review?(jryans)
Attachment #8756163 - Flags: ui-review?(hholmes) → ui-review+
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
Attachment #8756163 - Flags: review?(jryans) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/25566a14a7ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Iteration: --- → 49.3 - Jun 6
Priority: P3 → P1
Whiteboard: [multiviewport] [reserve-rdm] → [multiviewport] [mvp-rdm]
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?
Flags: needinfo?(jryans)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(jryans) → needinfo?(jaideepb)
Resolution: FIXED → ---
Iteration: 49.3 - Jun 6 → ---
Priority: P1 → P3
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [reserve-rdm]
Attached patch 1266411.patch [3.0] (obsolete) — Splinter Review
Attachment #8756163 - Attachment is obsolete: true
Attachment #8757582 - Flags: review+
Keywords: checkin-needed
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).
Keywords: checkin-needed
Attached patch 1266411.patch [4.0] (obsolete) — Splinter Review
Thanks for all the pointers. I'm uploading the patch with the changes that you said.
Attachment #8757582 - Attachment is obsolete: true
Attachment #8757668 - Flags: review?(jryans)
Status: REOPENED → ASSIGNED
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.
Attachment #8757668 - Flags: review?(jryans) → review-
Attached patch 1266411.patch [5.0] (obsolete) — Splinter Review
Attachment #8757668 - Attachment is obsolete: true
Flags: needinfo?(jaideepb)
Attachment #8758380 - Flags: review?(jryans)
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
Attachment #8758380 - Flags: review?(jryans) → review-
Attached patch 1266411.patch [6.0] (obsolete) — Splinter Review
Attachment #8758380 - Attachment is obsolete: true
Attachment #8758861 - Flags: review?(jryans)
Attached patch 1266411.patch [6.0] (obsolete) — Splinter Review
Attachment #8758861 - Attachment is obsolete: true
Attachment #8758861 - Flags: review?(jryans)
Attachment #8758867 - Flags: review?(jryans)
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.)
Attachment #8758867 - Flags: review?(jryans) → review+
Attachment #8758867 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/e3d9990ca6ec
Alphabetically sort devices in each type. r=jryans
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e3d9990ca6ec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Iteration: --- → 49.3 - Jun 6
Priority: P3 → P1
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.