Closed Bug 751904 Opened 12 years ago Closed 12 years ago

[responsive] design view GCLI commands

Categories

(DevTools :: General, defect, P2)

14 Branch
x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: dangoor, Assigned: miker)

References

Details

(Whiteboard: [gclicommands])

Attachments

(1 file, 4 obsolete files)

We should have commands to turn responsive design view on/off and to change the screen size. (Zoom and other options would be nice as well)
Target Milestone: --- → Firefox 16
Priority: -- → P2
Attached patch v0.999 (obsolete) — Splinter Review
Attached patch v1 (obsolete) — Splinter Review
Attachment #636646 - Attachment is obsolete: true
Attachment #636648 - Attachment description: v0.999 → v1
Attachment #636648 - Flags: review?(jwalker)
Maybe I should use a selection type? ("on" "off" "toggle")?
Comment on attachment 636648 [details] [diff] [review]
v1

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

I do think we need to add some tests for these commands. We've skipped it in the past and got bitten.
It's not hard, because there is a framework to help.

Examples:
- browser/devtools/commandline/test/browser_gcli_commands.js
- browser/devtools/commandline/test/browser_gcli_edit.js

There is even documentation (which I should perhaps move to MDN):
- browser/devtools/commandline/test/head.js
  See the doc comments for DeveloperToolbarTest.checkInputStatus()
  and DeveloperToolbarTest.exec()
Attachment #636648 - Flags: review?(jwalker)
Mike, do you mind taking over this?
Assignee: nobody → mratcliffe
Whiteboard: [gclicommands]
Blocks: GCLICMD
Attached patch v2 (obsolete) — Splinter Review
Now with tests
Attachment #636648 - Attachment is obsolete: true
Attachment #639673 - Flags: review?(jwalker)
Comment on attachment 639673 [details] [diff] [review]
v2

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

::: browser/devtools/commandline/GcliCommands.jsm
@@ +599,5 @@
> +                          this.name,
> +                          args);
> +  }
> +});
> +

We've got 2 commands in different namespaces here, which is going to make it hard to remember.

How about:

>> resize on
    # resizes to the default or whatever we were on last

>> resize off
    # obvious

>> resize toggle
    # obvious

>> resize to x y
    # obvious

?

::: browser/devtools/commandline/test/head.js
@@ +249,5 @@
> + */
> +DeveloperToolbarTest.checkInputStatusNow = function DTT_checkInputStatusNow(checks) {
> +  DeveloperToolbarTest.checkInputStatus(checks);
> +  DeveloperToolbarTest.exec();
> +};

I think the actual tests are missing?

I'm also not sure about this function. Generally exec will have checks that it makes, and this prevents us from making those checks.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +41,5 @@
> +   * @param aWindow the browser window.
> +   * @param aTab the tab targeted.
> +   * @param aArgs command arguments.
> +   */
> +  handleGcliCommand: function(aWindow, aTab, aCommand, aArgs) {

Nit: aCommand is missing from docs

Also I think we'd be better off having separate function rather than one function that does 2 totally different things based on a switch.
Attached patch v3 (obsolete) — Splinter Review
(In reply to Joe Walker from comment #8)
> Comment on attachment 639673 [details] [diff] [review]
> v2
> 
> Review of attachment 639673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +599,5 @@
> > +                          this.name,
> > +                          args);
> > +  }
> > +});
> > +
> 
> We've got 2 commands in different namespaces here, which is going to make it
> hard to remember.
> 
> How about:
> 
> >> resize on
>     # resizes to the default or whatever we were on last
> 
> >> resize off
>     # obvious
> 
> >> resize toggle
>     # obvious
> 
> >> resize to x y
>     # obvious
> 
> ?
> 

Done

> ::: browser/devtools/commandline/test/head.js
> @@ +249,5 @@
> > + */
> > +DeveloperToolbarTest.checkInputStatusNow = function DTT_checkInputStatusNow(checks) {
> > +  DeveloperToolbarTest.checkInputStatus(checks);
> > +  DeveloperToolbarTest.exec();
> > +};
> 
> I think the actual tests are missing?
> 

Yes, I hadn't added the file, added.

> I'm also not sure about this function. Generally exec will have checks that
> it makes, and this prevents us from making those checks.
> 

Agreed, removed

> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +41,5 @@
> > +   * @param aWindow the browser window.
> > +   * @param aTab the tab targeted.
> > +   * @param aArgs command arguments.
> > +   */
> > +  handleGcliCommand: function(aWindow, aTab, aCommand, aArgs) {
> 
> Nit: aCommand is missing from docs
> 

Added

> Also I think we'd be better off having separate function rather than one
> function that does 2 totally different things based on a switch.

Now that they are all resize commands hopefully it makes more sense ... it is certainly a lot easier to read and expand upon.
Attachment #639673 - Attachment is obsolete: true
Attachment #639673 - Flags: review?(jwalker)
Attachment #640214 - Flags: review?(jwalker)
Comment on attachment 640214 [details] [diff] [review]
v3

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

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +340,5 @@
> +
> +# LOCALIZATION NOTE (resizePageArgWidthDesc) A very short string to describe the
> +# 'width' parameter to the 'resizepage' command, which is displayed in a dialog
> +# when the user is using this command.
> +resizePageArgWidthDesc=width in pixels

l10n strings are hard to change, so it's worth doing some tweaks as we put this together: I think we need a capital 'W' (and 'H' in the next string)

@@ +350,5 @@
> +
> +# LOCALIZATION NOTE (resizeModeOnDesc) A very short string to describe the
> +# 'resizeon ' command. This string is designed to be shown in a menu
> +# alongside the command name, which is why it should be as short as possible.
> +resizeModeOnDesc=Turn the Responsive Design Mode on.

These strings need to fit in menus, so they should be short.
How about '[Enter|Exit|Toggle] Responsive Design Mode'?

@@ +365,5 @@
> +
> +# LOCALIZATION NOTE (resizeModeToDesc) A very short string to describe the
> +# 'resize to' command. This string is designed to be shown in a menu
> +# alongside the command name, which is why it should be as short as possible.
> +resizeModeToDesc=Change the Responsive Design Mode window size.

For brevity, how about 'Alter Responsive Design Mode size'

@@ +374,5 @@
> +resizeModeDesc=Control the Responsive Design Mode.
> +
> +# LOCALIZATION NOTE (resizeModeManual) A fuller description of the 'resize'
> +# command, displayed when the user asks for help on what it does.
> +resizeModeManual=Turn the Responsive Design Mode on and off.

We can afford to be as long as we like here, so how about:

"Responsive websites respond to their environment, so they look good on a mobile display and a cinema display and everything in-between. Responsive Design Mode allows you to easily test a variety of page sizes in Firefox without needing to resize your whole browser"
Attachment #640214 - Flags: review?(jwalker) → review+
s/Alter Responsive Design Mode size/Alter page size ?

Beside that, everything looks fine.
Attached patch v4Splinter Review
Done
Attachment #640214 - Attachment is obsolete: true
Added to master patch in bug 768998
Copy / paste error ... added to master patch in but 771555.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: