Closed Bug 1321675 Opened 8 years ago Closed 7 years ago

Minimal UI to save frequently used viewport sizes

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Version:
49 Branch
Type:
defect

Tracking

(relnote-firefox 54+, firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
relnote-firefox --- 54+
firefox54 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [rdm-v2])

Attachments

(7 files)

Bug 1321675 - Add form to device modal for adding devices.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1321675 - Show device details in tooltip.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1321675 - Rename removeDevice to removeDeviceAssoc.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1321675 - Add / remove custom devices in RDM.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1321675 - Move device adder below devices.
59 bytes, text/x-review-board-request
gl
: review+
Details
add_device.png,
78.46 KB, image/png
Details
Print screen from Ubuntu.
88.77 KB, image/png
Details 
One feature missing from new RDM compared to old RDM is there's currently no way to save viewport sizes.  From the comments on the new RDM Hacks post, some people used the saving feature to store a few sizes matching each of the responsive breakpoints on their site, so they can jump to each one quickly.

We do plan to offer custom device creation with all properties of a device (bug 1173142), but maybe there's something smaller we can do to bring back saving of sizes at least.

We'll need to work out the right UX for this.

Possibly one of these:

A. Add button in the device modal with size inputs
B. Save button next to dimension inputs near viewport (would need somewhere to input name?)
Helen, any thoughts on the right design here?
Flags: needinfo?(hholmes)
Flags: needinfo?(hholmes)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
After discussing with :clarkbw, we agreed that the simplest thing here is to provide a custom device UI.  I'm adding it to the device modal dialog.

It will be just enough to add a device and remove them, nothing more.
Comment on attachment 8834229 [details]
Bug 1321675 - Add form to device modal for adding devices.

https://reviewboard.mozilla.org/r/110230/#review111378

::: devtools/client/responsive.html/components/device-adder.js:67
(Diff revision 1)
> +          getStr("responsive.addDevice")
> +        )
> +      );
> +    }
> +
> +    // If a device is currently selected, fold its attributes into single object for use

s/into/into a

::: devtools/client/responsive.html/components/device-adder.js:71
(Diff revision 1)
> +
> +    // If a device is currently selected, fold its attributes into single object for use
> +    // as the starting values of the form.  If no device is selected, use the values for
> +    // the current window.
> +    let deviceName;
> +    let normalizedViewport = Object.assign({}, viewportTemplate);

normalizedViewport and viewportTemplate are very weird name and was probably the hardest thing for me to wrap my head around. This is really just an object that contains the viewports or devices pixel ratio, ua, touch, etc. 

I would prefer if we can be a bit more explicit and maybe rename viewportTemplate to deviceAdderViewportTemplate.

::: devtools/client/responsive.html/components/device-adder.js:73
(Diff revision 1)
> +    // as the starting values of the form.  If no device is selected, use the values for
> +    // the current window.
> +    let deviceName;
> +    let normalizedViewport = Object.assign({}, viewportTemplate);
> +    if (viewportTemplate.device) {
> +      let device = devices[viewportTemplate.deviceType].find(elem => {

s/elem/device to be more clear since these aren't actually elements.

::: devtools/client/responsive.html/components/device-adder.js:158
(Diff revision 1)
> +          },
> +          getStr("responsive.deviceAdderTouch")
> +        ),
> +        dom.input({
> +          type: "checkbox",
> +          defaultChecked: normalizedViewport.touch,

I would prefer all props to be alphabetically ordered with the exception of the handler functions. Perhaps, swapping defaultChecked and type.

::: devtools/client/responsive.html/index.css:370
(Diff revision 1)
>  }
>  
>  .viewport-dimension-editable.editing,
>  .viewport-dimension-input.editing {
>    color: var(--viewport-active-color);
> +  outline: none;

I have considered and maybe removed outline in the past for thie dimension input, but I think the outline is kept for accessibility. I am not fully sure of the value of the outline and always thought it was incredibly ugly for the dimension input.

::: devtools/client/responsive.html/index.css:526
(Diff revision 1)
>    background-color: var(--submit-button-active-background-color);
>    color: var(--submit-button-active-color);
>  }
> +
> +/**
> + * Device Add

I think we should just stick with the naming and call this Device Adder.
Attachment #8834229 - Flags: review?(gl) → review+
Comment on attachment 8834230 [details]
Bug 1321675 - Show device details in tooltip.

https://reviewboard.mozilla.org/r/110232/#review111668
Attachment #8834230 - Flags: review?(gl) → review+
Comment on attachment 8834231 [details]
Bug 1321675 - Rename removeDevice to removeDeviceAssoc.

https://reviewboard.mozilla.org/r/110234/#review111670
Attachment #8834231 - Flags: review?(gl) → review+
Comment on attachment 8834232 [details]
Bug 1321675 - Add / remove custom devices in RDM.

https://reviewboard.mozilla.org/r/110236/#review111674

::: devtools/client/responsive.html/actions/devices.js:78
(Diff revision 1)
>  
>    updatePreferredDevices: updatePreferredDevices,
>  
> +  addCustomDevice(device) {
> +    return function* (dispatch) {
> +      // Add custom device to device storage

Adds a custom device to the device storage

::: devtools/client/responsive.html/actions/devices.js:162
(Diff revision 1)
>  
>            dispatch(module.exports.addDevice(newDevice, type));
>          }
>        }
>  
> +      // Add an empty "custom" type if it doesn't exist in device storage

s/Add/Adds

::: devtools/client/responsive.html/components/device-adder.js:61
(Diff revision 1)
>    onDeviceAdderSave() {
> +    let {
> +      devices,
> +      onAddCustomDevice,
> +    } = this.props;
> +    if (!this.pixelRatioInput.checkValidity()) {

We should add a new line after this.props;

::: devtools/client/responsive.html/components/device-adder.js:64
(Diff revision 1)
> +      onAddCustomDevice,
> +    } = this.props;
> +    if (!this.pixelRatioInput.checkValidity()) {
> +      return;
> +    }
> +    if (devices.custom.find(device => device.name == this.nameInput.value)) {

Add a new line after the last if statement block

::: devtools/client/responsive.html/components/device-adder.js:68
(Diff revision 1)
> +    }
> +    if (devices.custom.find(device => device.name == this.nameInput.value)) {
> +      this.nameInput.setCustomValidity("Device name already in use");
> +      return;
> +    }
>      this.setState({

Add a new line after the last if statement block

::: devtools/client/responsive.html/components/device-adder.js:148
(Diff revision 1)
>            },
>            getStr("responsive.deviceAdderName")
>          ),
>          dom.input({
>            defaultValue: deviceName,
> +          ref: input => {

We can do ref: "nameInput" instead. Otherwise, do we have a preference for doing it this way? I can see the advantage of not having to do this.refs.nameInput, but that's it.

::: devtools/client/responsive.html/components/device-adder.js:186
(Diff revision 1)
>          ),
>          dom.input({
> +          type: "number",
> +          step: "any",
>            defaultValue: normalizedViewport.pixelRatio,
> +          ref: input => {

ref: "pixelRatioInput"

::: devtools/client/responsive.html/components/device-adder.js:221
(Diff revision 1)
>            getStr("responsive.deviceAdderTouch")
>          ),
>          dom.input({
>            type: "checkbox",
>            defaultChecked: normalizedViewport.touch,
> +          ref: input => {

ref: "touchInput"

::: devtools/client/responsive.html/index.css:500
(Diff revision 1)
>    font-size: 11px;
>    padding-bottom: 3px;
>    display: flex;
>    align-items: center;
> +  /* Largest size without horizontal scrollbars */
> +  max-width: 181px;

I am not too sure about this since we still get the horizontal scrollbars for a really long device name.

::: devtools/client/responsive.html/reducers/devices.js:78
(Diff revision 1)
>  
> +  [REMOVE_DEVICE](devices, { device, deviceType }) {
> +    let index = devices[deviceType].indexOf(device);
> +    if (index < 0) {
> +      return devices;
> +    }

Add a new line after this if statement block

::: devtools/client/shared/devices.js:12
(Diff revision 1)
>  const { getJSON } = require("devtools/client/shared/getjson");
>  
>  const DEVICES_URL = "devtools.devices.url";
>  const { LocalizationHelper } = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/client/locales/device.properties");
> +const { Task } = require("devtools/shared/task");

The requires and constants declaration could use some formatting. I would Task to move above { getJSON }.

We should move DEVICES_URL const after LOCAL_DEVICES.
Attachment #8834232 - Flags: review?(gl) → review+
I felt that we should be sticking to Helen's UX design for the device adder form, but I am happy with the changes that were added so far that we can iterate on this. 

One of the reasons why I am advocating for sticking with the ux design was because the UA input was too small, and can benefit from having a larger form space.
I think I just completely forgot there were any existing designs for adding devices.  Anyway, :gl gave me a link to Helen's design:

https://projects.invisionapp.com/share/ZHACVTXR2#/screens/137978120
Comment on attachment 8834229 [details]
Bug 1321675 - Add form to device modal for adding devices.

https://reviewboard.mozilla.org/r/110230/#review111378

> normalizedViewport and viewportTemplate are very weird name and was probably the hardest thing for me to wrap my head around. This is really just an object that contains the viewports or devices pixel ratio, ua, touch, etc. 
> 
> I would prefer if we can be a bit more explicit and maybe rename viewportTemplate to deviceAdderViewportTemplate.

Okay, I renamed it to `deviceAdderViewportTemplate` in the compoents above the adder.

> s/elem/device to be more clear since these aren't actually elements.

`device` can't be used here because it's already used in the outer scope (blocked by linting rules).  I guess I'll change to `d`.

> I would prefer all props to be alphabetically ordered with the exception of the handler functions. Perhaps, swapping defaultChecked and type.

Okay, changed.

> I have considered and maybe removed outline in the past for thie dimension input, but I think the outline is kept for accessibility. I am not fully sure of the value of the outline and always thought it was incredibly ugly for the dimension input.

I think it's safe to remove outline here, since we have a different visual treatment when something is focused (the bottom border).
Comment on attachment 8834232 [details]
Bug 1321675 - Add / remove custom devices in RDM.

https://reviewboard.mozilla.org/r/110236/#review111674

> Adds a custom device to the device storage

I don't think I see the grammar issue here...  It seems fine for a line comment.  It's describing the task of the next line.

> s/Add/Adds

Same, grammar seems fine to me.

> We can do ref: "nameInput" instead. Otherwise, do we have a preference for doing it this way? I can see the advantage of not having to do this.refs.nameInput, but that's it.

I used to use string refs in the past too.  According to React docs though, this is now deprecated[1] and ref callbacks are suggested instead.

[1]: https://facebook.github.io/react/docs/refs-and-the-dom.html#legacy-api-string-refs

> I am not too sure about this since we still get the horizontal scrollbars for a really long device name.

Are you sure?  It doesn't seem to happen for me, at least on macOS.  The device name wraps to multiple lines if it's really long.

> The requires and constants declaration could use some formatting. I would Task to move above { getJSON }.
> 
> We should move DEVICES_URL const after LOCAL_DEVICES.

Okay, I attempted to reach a better order. :)
Release Note Request (optional, but appreciated)
[Why is this notable]: You can add custom devices to RDM in DevTools
[Affects Firefox for Android]: No
[Suggested wording]: Create your own custom devices in Responsive Device Mode
[Links (documentation, blog post, etc)]: MDN docs to be updated...
relnote-firefox: --- → ?
Flags: qe-verify+
Keywords: dev-doc-needed
Comment on attachment 8834657 [details]
Bug 1321675 - Move device adder below devices.

https://reviewboard.mozilla.org/r/110520/#review112116

This is getting closer to Helen's design, but I am still expecting the full mockup to be implemented in the next iteration. The "Add Device" to see the form fields and the large amount of empty space that is taken up by the single button is a bit unsightly. Nonetheless, I believe this is an iterative process and  happy with the changes so far.

::: devtools/client/responsive.html/components/device-adder.js:144
(Diff revision 1)
> +        {
> +          id: "device-adder-content",
> +        },
> +        dom.div(
> +          {
> +            id: "device-adder-column-1"

Add the comma the end for consistency.

::: devtools/client/responsive.html/components/device-adder.js:204
(Diff revision 1)
>  <span class="indent">>>>></span>        })
> +          )
> -      ),
> +        ),
> +        dom.div(
> +          {
> +            id: "device-adder-column-2"

Same as above
Attachment #8834657 - Flags: review?(gl) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/856316b88fc1
Add form to device modal for adding devices. r=gl
https://hg.mozilla.org/integration/autoland/rev/87f892b0cc25
Show device details in tooltip. r=gl
https://hg.mozilla.org/integration/autoland/rev/64e6b898e491
Rename removeDevice to removeDeviceAssoc. r=gl
https://hg.mozilla.org/integration/autoland/rev/14d0c91bab4e
Add / remove custom devices in RDM. r=gl
https://hg.mozilla.org/integration/autoland/rev/c648ae80e1df
Move device adder below devices. r=gl
Hi Ryan,

Can you please advice me how can I test this fix? If you have test case or STR it would help. Thanks
Flags: needinfo?(jryans)
(In reply to ovidiu boca[:Ovidiu] from comment #26)
> Hi Ryan,
> 
> Can you please advice me how can I test this fix? If you have test case or
> STR it would help. Thanks

STR 1:

1. Go to some page
2. Open RDM
3. In the device selector above the viewport, choose "Edit list" to show the device modal
4. Click "Add Device" to create a custom device
5. The form should be pre-filled with the current viewport size and other properties like UA, DPR, etc. are set to Firefox defaults on your computer
6. Change the name and properties as desired and save
7. Check the checkbox next to the new device so it will appear in selector
  * Custom devices should probably start enabled by default, I filed bug 1338599 for this
8. Close device modal with "Done" button at bottom
9. In device selector, choose your new custom device
10. Device properties like navigator.userAgent and devicePixelRatio should match what you entered

STR 2:

1. Go to some page
2. Open RDM
3. Choose an existing device in the device selector
4. In the device selector above the viewport, choose "Edit list" to show the device modal
5. Click "Add Device" to create a custom device
6. The form should be pre-filled with the properties of the device you selected in step 3
Flags: needinfo?(jryans)
Attached image add_device.png, — 
Hi Ryan,

Thanks for adding steps. I verified this issue on Mac 10.10, Windows 10 and Ubuntu 16.04 with FF Nighlty 54.0a1(2017-02-12) and I still can reproduce the issues from bug 1338599 and bug 1338604. I also can confirm that scenarios from comment 27 are working. 
On Ubuntu and Windows I saw a problem regarding the "Save" and "Done" buttons, they are on top of each other. Please see the attached print screens. 

I have one scenario that I think is not working as expected, but you can decide on that:

1. Go to some page
2. Open RDM
3. In the device selector above the viewport, choose "Edit list" to show the device model
4. Click "Add Device" to create a custom device
5. If you don't want to create a custom device and you exit from this menu and enter again in the Edit List the "Custom Device" menu is already opened from the last time. I think the expected result here should be to have the "Add Device" button. Please tell me your opinion about this.
Thanks for testing!

(In reply to ovidiu boca[:Ovidiu] from comment #28)
> Created attachment 8836640 [details]
> add_device.png,
> 
> Hi Ryan,
> 
> Thanks for adding steps. I verified this issue on Mac 10.10, Windows 10 and
> Ubuntu 16.04 with FF Nighlty 54.0a1(2017-02-12) and I still can reproduce
> the issues from bug 1338599 and bug 1338604. I also can confirm that
> scenarios from comment 27 are working. 
> On Ubuntu and Windows I saw a problem regarding the "Save" and "Done"
> buttons, they are on top of each other. Please see the attached print
> screens. 

Good catch, I filed bug 1339155 to improve the layout on those platforms.

> I have one scenario that I think is not working as expected, but you can
> decide on that:
> 
> 1. Go to some page
> 2. Open RDM
> 3. In the device selector above the viewport, choose "Edit list" to show the
> device model
> 4. Click "Add Device" to create a custom device
> 5. If you don't want to create a custom device and you exit from this menu
> and enter again in the Edit List the "Custom Device" menu is already opened
> from the last time. I think the expected result here should be to have the
> "Add Device" button. Please tell me your opinion about this.

I agree it's a bit strange, yes.  Please file a new bug for this.
Blocks: 1339358
No longer blocks: 1339358
Depends on: 1339358
Hi,

I filed a new bug 1339358 like you suggested, and I marked it as depends of this issue, sorry for the spam. 

Based on the fact that I can't reproduce the scenarios from comment 27 I think I will change the tracking flag for this issue into verified. The entire bug 1321675 will be mark as verified after all the bugs from depends and blocks are fixed and verified. Please tell me what do you think about this.
Flags: needinfo?(jryans)
(In reply to ovidiu boca[:Ovidiu] from comment #31)
> Hi,
> 
> I filed a new bug 1339358 like you suggested, and I marked it as depends of
> this issue, sorry for the spam. 
> 
> Based on the fact that I can't reproduce the scenarios from comment 27 I
> think I will change the tracking flag for this issue into verified. The
> entire bug 1321675 will be mark as verified after all the bugs from depends
> and blocks are fixed and verified. Please tell me what do you think about
> this.

Thanks for filing!  I think it's fine to mark this bug as verified now, since the overall feature works, and the known issues are filed separately, so it would seem like the verification work here is done.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(jryans)
Added to Fx54 (Aurora) release notes.
relnote-firefox: ? → 54+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: