Closed Bug 1333254 Opened 7 years ago Closed 7 years ago

Adjust spelling of device pixel ratio

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox54 affected, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox54 --- affected
firefox59 --- fixed

People

(Reporter: jryans, Assigned: leakey94, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [rdm-v2])

Attachments

(1 file, 2 obsolete files)

Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
We currently have lots of different spellings of "device pixel ratio" in both code and localized strings:

* DevicePixelRatio
* devicePixelRatio
* Device Pixel Ratio
* DPR

The DOM property is `window.devicePixelRatio`.  It is not generally treated as a proper noun, so "device pixel ratio" seems like the best spelling to describe the value.

We should clean this up in the RDM code base.
I think seems like a good first bug.

Search for such strings in places like:

/devtools/client/responsive.html
/devtools/client/locales/en-US/responsive.properties
/devtools/server/actors

If you need help getting started, check out http://docs.firefox-dev.tools/getting-started/.

If you have any questions, please ask them here and set the needinfo? field to mentor.
Mentor: jryans
Keywords: good-first-bug
Hi, I'd like to take this on. What's the best way to find the specific locations of the use of "DevicePixelRatio" or similar terms?
Flags: needinfo?(jryans)
(In reply to Ryan Leake from comment #2)
> Hi, I'd like to take this on. What's the best way to find the specific
> locations of the use of "DevicePixelRatio" or similar terms?

Thanks for your interest!

One option is to use Searchfox focused to /devtools:

http://searchfox.org/mozilla-central/search?q=DevicePixelRatio&case=false&regexp=false&path=devtools

Once you have the source locally, you could use whatever local search tool you like to look in those directories.

I believe most usages in code are already correct, but there are various comments and string IDs for translation that would be good to correct.

In general, I think we want to aim for:

* `devicePixelRatio` in code and string IDs
* "device pixel ratio" in comments and other non-code text

Let me know if you need more details about any portion of this!
Flags: needinfo?(jryans)
I'm a student at Seneca college learning open source, and I was hoping to work on this bug for my course.  If no one else is currently working on it, I'd like to give it a try, but before i would like to ask a couples of question

1:comments like "DevicePixelRatio (DPR) dropdown when is enabled." 
      should be "device pixel ratio(DPR) dropdown when is enabled."

2:code like "responsive.devicePixelRatio=Device Pixel Ratio" 
    should be "responsive.devicePixelRatio=devicePixelRatio"
(In reply to ewhite7 from comment #4)
> I'm a student at Seneca college learning open source, and I was hoping to
> work on this bug for my course.  If no one else is currently working on it,
> I'd like to give it a try, but before i would like to ask a couples of
> question

Let's check if Ryan still plans to do so.

> 1:comments like "DevicePixelRatio (DPR) dropdown when is enabled." 
>       should be "device pixel ratio(DPR) dropdown when is enabled."

It's probably fine to remove the DPR as well, so "device pixel ratio dropdown when is enabled."

> 2:code like "responsive.devicePixelRatio=Device Pixel Ratio" 
>     should be "responsive.devicePixelRatio=devicePixelRatio"

The value here is a string used in the tooltip when hovering the devicePixelRatio menu in RDM, so this might be a bit of a special case.

I'd suggest starting the text with a verb, to match other tooltips, so something like "Change device pixel ratio of the viewport".

This bug is pretty subjective in general, so there might be a round of back and forth during review.  Thanks for your interest!
Flags: needinfo?(leakey94)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> (In reply to ewhite7 from comment #4)
> > I'm a student at Seneca college learning open source, and I was hoping to
> > work on this bug for my course.  If no one else is currently working on it,
> > I'd like to give it a try, but before i would like to ask a couples of
> > question
> 
> Let's check if Ryan still plans to do so.
> 
> > 1:comments like "DevicePixelRatio (DPR) dropdown when is enabled." 
> >       should be "device pixel ratio(DPR) dropdown when is enabled."
> 
> It's probably fine to remove the DPR as well, so "device pixel ratio
> dropdown when is enabled."
> 
> > 2:code like "responsive.devicePixelRatio=Device Pixel Ratio" 
> >     should be "responsive.devicePixelRatio=devicePixelRatio"
> 
> The value here is a string used in the tooltip when hovering the
> devicePixelRatio menu in RDM, so this might be a bit of a special case.
> 
> I'd suggest starting the text with a verb, to match other tooltips, so
> something like "Change device pixel ratio of the viewport".
> 
> This bug is pretty subjective in general, so there might be a round of back
> and forth during review.  Thanks for your interest!

Just confirming I'm still working on this.
Flags: needinfo?(leakey94)
In cases where the string is referring directly to the variable, for example:

// Scale down by `devicePixelRatio` since SVG element already take them into
x: Math.round(point.x / devicePixelRatio)

should 'devicePixelRatio' be left as is, or changed to device pixel ratio?

Thanks!
Flags: needinfo?(jryans)
(In reply to Ryan Leake from comment #7)
> In cases where the string is referring directly to the variable, for example:
> 
> // Scale down by `devicePixelRatio` since SVG element already take them into
> x: Math.round(point.x / devicePixelRatio)
> 
> should 'devicePixelRatio' be left as is, or changed to device pixel ratio?

Since that comment is using backticks to explicitly mention a variable (or potentially expression in other examples) specifically, I think it's best to leave it as is.
Flags: needinfo?(jryans)
Attachment #8925240 - Flags: review?(jryans)
Comment on attachment 8925240 [details]
Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.

https://reviewboard.mozilla.org/r/196450/#review202968

Thanks for working on this! :) You're making good progress.

For the next round, please add a comment message[1] that describes the change.

I think we should also rename the file `devtools/client/responsive.html/components/DprSelector.js` to `DevicePixelRatioSelector.js`.  This component contains various other usages of `DPR` that are probably worth looking at, like element IDs.

Overall, take look at this search for `DPR` in DevTools, as I think it reveals a few more spots to clean up:

http://searchfox.org/mozilla-central/search?q=DPR&case=true&regexp=false&path=devtools

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

::: devtools/client/locales/en-US/responsive.properties:78
(Diff revision 1)
>  # so a short string would be best if possible.
>  responsive.noThrottling=No throttling
>  
>  # LOCALIZATION NOTE (responsive.devicePixelRatio): tooltip for the
> -# DevicePixelRatio (DPR) dropdown when is enabled.
> -responsive.devicePixelRatio=Device Pixel Ratio
> +# device pixel ratio dropdown when is enabled.
> +responsive.devicePixelRatio=Change device pixel ratio of the viewport.

When changing string values like this, we also have to alter the string ID for our translation tools to handle it properly.

So, something like change `responsive.devicePixelRatio` to something else such as `responsive.changeDevicePixelRatio`, and update any code references to use the new ID.

Also, tooltips shouldn't end with periods.  (I have done this incorrectly numerous times myself, that's the only reason I know!)

Here's a long guide on strings that includes these and other tips:

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

::: devtools/client/locales/en-US/responsive.properties:81
(Diff revision 1)
>  # LOCALIZATION NOTE (responsive.devicePixelRatio): tooltip for the
> -# DevicePixelRatio (DPR) dropdown when is enabled.
> -responsive.devicePixelRatio=Device Pixel Ratio
> +# device pixel ratio dropdown when is enabled.
> +responsive.devicePixelRatio=Change device pixel ratio of the viewport.
>  
> -# LOCALIZATION NOTE (responsive.autoDPR): tooltip for the DevicePixelRatio
> -# (DPR) dropdown when is disabled because a device is selected.
> +# LOCALIZATION NOTE (responsive.autoDPR): tooltip for the device pixel ratio
> +# dropdown when is disabled because a device is selected.

Let's also fix "when is disabled" -> "when it is disabled" while we're here.

::: devtools/client/locales/en-US/responsive.properties:84
(Diff revision 1)
>  
> -# LOCALIZATION NOTE (responsive.autoDPR): tooltip for the DevicePixelRatio
> -# (DPR) dropdown when is disabled because a device is selected.
> +# LOCALIZATION NOTE (responsive.autoDPR): tooltip for the device pixel ratio
> +# dropdown when is disabled because a device is selected.
>  # The argument (%1$S) is the selected device (e.g. iPhone 6) that set
>  # automatically the DPR value.
>  responsive.autoDPR=DPR automatically set by %1$S

Let's also get the `DPR` out of this string ID and value.

So, something like:

```
responsive.devicePixelRatio.auto=Device pixel ratio automatically set by %1$S
```

The code will also need to change for the new string ID.

::: devtools/client/locales/en-US/responsive.properties:114
(Diff revision 1)
>  
>  # LOCALIZATION NOTE (responsive.deviceAdderPixelRatio): Label of form field for
> -# the devicePixelRatio of a new device.  The available width is very low, so you
> +# the device pixel ratio of a new device.  The available width is very low, so you
>  # might see overlapping text if the length is much longer than 5 or so
>  # characters.
>  responsive.deviceAdderPixelRatio=DPR

It would be nice to change this string value, but at the moment it would break the device adder form layout.

For bonus points, you could add a `title` attribute for this label[1], so it can show the "Device pixel ratio" in a tooltip.  (You'd also need a new string for the text.)

Just an idea though, not required!

[1]: http://searchfox.org/mozilla-central/source/devtools/client/responsive.html/components/DeviceAdder.js#184

::: devtools/client/locales/en-US/responsive.properties:136
(Diff revision 1)
>  # LOCALIZATION NOTE (responsive.deviceDetails): Tooltip that appears when
>  # hovering on a device in the device modal.  %1$S is the width of the device.
> -# %2$S is the height of the device.  %3$S is the devicePixelRatio value of the
> +# %2$S is the height of the device.  %3$S is the device pixel ratio value of the
>  # device.  %4$S is the user agent of the device.  %5$S is a boolean value
>  # noting whether touch input is supported.
>  responsive.deviceDetails=Size: %1$S x %2$S\nDPR: %3$S\nUA: %4$S\nTouch: %5$S

I almost said let's change this too, but probably it's best to leave as-is, since it matches the device adder form, and the label there is short because of the small width.
Attachment #8925240 - Flags: review?(jryans) → review-
Thanks for the detailed response, it's super helpful!
Just a few questions.

In GlobalToolbar.js, should:

     const DPRSelector = createFactory(require("./DprSelector"));
    
be changed to?

     const DevicePixelRatioSelector = createFactory(require("./DevicePixelRatioSelector"));

(as DprSelector.js has been renamed to DevicePixelRatioSelector.js).

Can you advise on how to modify the code below to remove references to DPR?

     return dom.label(
           {
             id: "global-dpr-selector",
             className: selectorClass,
             title,
           },
           "DPR",

Many thanks.
Flags: needinfo?(jryans)
(In reply to Ryan Leake from comment #11)
> Thanks for the detailed response, it's super helpful!
> Just a few questions.
> 
> In GlobalToolbar.js, should:
> 
>      const DPRSelector = createFactory(require("./DprSelector"));
>     
> be changed to?
> 
>      const DevicePixelRatioSelector =
> createFactory(require("./DevicePixelRatioSelector"));
> 
> (as DprSelector.js has been renamed to DevicePixelRatioSelector.js).

Yes, that change sounds reasonable to me.

> Can you advise on how to modify the code below to remove references to DPR?
> 
>      return dom.label(
>            {
>              id: "global-dpr-selector",
>              className: selectorClass,
>              title,
>            },
>            "DPR",
> 
> Many thanks.

I'd suggest changing the ID to "global-device-pixel-ratio-selector".  You'll then also need to update the CSS as well:

https://searchfox.org/mozilla-central/rev/02bc7e2b1bcbab2d7b604ca713b42bcefd47ad2d/devtools/client/responsive.html/index.css#179-212

The literal text "DPR" should probably remain, as that's displayed in the UI and saves screen space, and it already uses a tooltip to clarify (which I believe will updated by your patch).
Flags: needinfo?(jryans)
Comment on attachment 8927570 [details]
[mq]: Bug1333254

https://reviewboard.mozilla.org/r/198870/#review204216

I believe all the code changes look great. :)

However, our usual workflow is to squash revised versions of a patch together, instead of stack new commits on top like this.  (For large changes, breaking it up into several, self-contained commits[1] can be a good idea.  But for the work we're doing here, probably a single commit is sufficient.)

Ideally, we'll end up with a single commit with all your work for this bug, including a correct recording of the file move because of renaming.  (This can be tricky to get right sometimes.  If you're using Mercurial, double-check that your config includes `git = true` for diffs.)  

[1]: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#prefer-more-smaller-commits-over-large-monolithic-commits

::: tags.txt:1
(Diff revision 1)
> +tip                            387105:87e3813e7939

I am not sure how this file got here, but please strip it out of the commit.
Attachment #8927570 - Flags: review?(jryans) → review-
Comment on attachment 8927571 [details]
Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.

https://reviewboard.mozilla.org/r/198872/#review204218

These changes also look good, and the commit message seems appropriate.

So, the remaining work here is to squash these 3 commits together, preserving the renaming info.

If you're not sure how to do that, feel free to join Mozilla IRC (such as #introduction) and ask!
Attachment #8927571 - Flags: review?(jryans) → review-
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Comment on attachment 8927571 [details]
> Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.
> 
> https://reviewboard.mozilla.org/r/198872/#review204218
> 
> These changes also look good, and the commit message seems appropriate.
> 
> So, the remaining work here is to squash these 3 commits together,
> preserving the renaming info.
> 
> If you're not sure how to do that, feel free to join Mozilla IRC (such as
> #introduction) and ask!

I'm having trouble squashing the commits in Mercurial, so if I were to start from fresh, restore the codebase and made identical changes and commit as a single patch would that replace the existing stacked commits?
Flags: needinfo?(jryans)
(In reply to Ryan Leake from comment #17)
> I'm having trouble squashing the commits in Mercurial, so if I were to start
> from fresh, restore the codebase and made identical changes and commit as a
> single patch would that replace the existing stacked commits?

Well, it should work, but let's think of that as a last resort!  We should definitely be able to get Mercurial to do what we need here.

Here some additional pages which may help:

https://www.mercurial-scm.org/wiki/ConcatenatingChangesets
https://stackoverflow.com/questions/1725607/can-i-squash-commits-in-mercurial

I haven't used Mercurial in a while myself, so I don't have a specific solution to offer, unfortunately.  If you haven't tried it yet, I would still suggest the #introduction room on Mozilla IRC.  If you aren't familiar with IRC, here are some tips on that as well:

https://wiki.mozilla.org/IRC#Connect_to_the_Mozilla_IRC_server
Flags: needinfo?(jryans)
Attachment #8927570 - Attachment is obsolete: true
Attachment #8927571 - Attachment is obsolete: true
Comment on attachment 8925240 [details]
Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.

https://reviewboard.mozilla.org/r/196450/#review205172

Have you been able to build your changes and run tests along the way?

Here are some guides on each topic:

http://docs.firefox-dev.tools/getting-started/build.html
http://docs.firefox-dev.tools/tests/

For running tests, I would expect that:

```
./mach test devtools/client/responsive.html
```

would cover all the affected tests for this patch.

It looks like the `browser_dpr_change.js` test fails.  It may need to be updated for the new selector ID.  If you like, you could also rename that test to fit the new naming we've been using in this bug.  If you rename the test, you'll also need to update the browser.ini file in the same directory to match the new file name.

::: devtools/client/responsive.html/components/moz.build:12
(Diff revision 2)
>  DevToolsModules(
>      'Browser.js',
>      'DeviceAdder.js',
>      'DeviceModal.js',
>      'DeviceSelector.js',
> -    'DprSelector.js',
> +    'DevicePixelRatioSelector.js',

When trying to build with this patch applied, I get an error about this list now being unsorted, so you'll need to move this up one line now that it has been renamed.
Attachment #8925240 - Flags: review?(jryans) → review-
Assignee: nobody → leakey94
Status: NEW → ASSIGNED
Comment on attachment 8925240 [details]
Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.

https://reviewboard.mozilla.org/r/196450/#review206030

Thanks for your continued efforts here! :)

I think think version looks ready to land!  I noticed that bug 1319597 (which just landed today) would conflict with some of the files here, so I'll help out and resolve those conflicts locally before pushing this change.
Attachment #8925240 - Flags: review?(jryans) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fef2737254a5
Adjust variations of 'device pixel ratio' spelling. r=jryans
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> Comment on attachment 8925240 [details]
> Bug 1333254 - Adjust variations of 'device pixel ratio' spelling.
> 
> https://reviewboard.mozilla.org/r/196450/#review206030
> 
> Thanks for your continued efforts here! :)
> 
> I think think version looks ready to land!  I noticed that bug 1319597
> (which just landed today) would conflict with some of the files here, so
> I'll help out and resolve those conflicts locally before pushing this change.

No problem, thanks for your help (and patience)!
https://hg.mozilla.org/mozilla-central/rev/fef2737254a5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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: