Closed
Bug 1297431
Opened 8 years ago
Closed 6 years ago
Add a custom user agent input in responsive design mode
Categories
(DevTools :: Responsive Design Mode, defect, P3)
DevTools
Responsive Design Mode
Tracking
(firefox64 fixed)
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: ntim, Assigned: gl)
References
Details
(Whiteboard: [multiviewport] [reserve-rdm] [ux])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
rcaliman
:
review+
|
Details | Diff | Splinter Review |
Currently, we silently change the UA on device change, we should be more obvious about it.
Also, the user should be able to change the UA independently from the device.
Updated•8 years ago
|
Whiteboard: [multiviewport] [triage]
I imagine actually changing to custom non-device UA would be separate work. So for this bug, we should focus on:
* potentially communicating that UA is changed to device's UA, but needs reload to fully affect the page
* potentially reminding the user somehow that a special UA is currently applied while a device is selected
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [multiviewport][triage][ux] → [multiviewport] [reserve-rdm] [ux]
Helen, any thoughts on this topic?
Flags: needinfo?(hholmes)
Comment 3•8 years ago
|
||
I think that in the case that a user changes device and the UA changes it's not important to communicate to the user the change. I think we should only worry about communicating UA changed in two instances:
1. The user changed the UA themselves, and has a custom UA.
2. The user had a custom UA, and then changed device, overriding their custom UA.
I think otherwise surfacing this kind of information to users who either don't care/don't understand what UA is is doing them a disservice.
Flags: needinfo?(hholmes)
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 4•6 years ago
|
||
https://mozilla.invisionapp.com/share/HQNDP3OZU8W#/
- Idea for a quick MVP to be hidden behind a flag
- Potential fancier idea to develop later we need it - just filing it here to keep things together
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gl
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Summary: [UX] Make it more obvious that an UA is applied in new RDM → Add a custom user agent input in responsive design mode
Assignee | ||
Comment 5•6 years ago
|
||
This adds a custom user agent input to the toolbar that is currently only enabled by default in nightly. Otherwise, this is toggle-able via the settings menu in the toolbar.
This also shows any of the applied UA by the device selector.
Attachment #9007491 -
Flags: review?(rcaliman)
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9007491 -
Attachment is obsolete: true
Attachment #9007491 -
Flags: review?(rcaliman)
Attachment #9007492 -
Flags: review?(rcaliman)
Assignee | ||
Comment 8•6 years ago
|
||
:karlcow, this might be of high interest to you. So, I am needinfoing you in case you want to take an early try at it and put any immediate feedback so I can continue to iterate on it. Otherwise, expect it in Nightly hopefully this week.
Flags: needinfo?(kdubost)
Reporter | ||
Comment 9•6 years ago
|
||
Thanks for working on this! Works pretty nicely, one thing though, the 900px input on focus doesn’t really work well on smaller windows.
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #9)
> Thanks for working on this! Works pretty nicely, one thing though, the 900px
> input on focus doesn’t really work well on smaller windows.
Ya, I was mainly playing with this width to kinda figure out how long does it really need to be to see the entire user agent string and how much to display when there is actually a UA string applied and not focused. I could imagine a text area solution but that would enlarge the toolbar.
Comment 11•6 years ago
|
||
It just occurred to me that it would be cool if the width of the input was flexible to fill up remaining space, maybe with 10% left/right-margin built in so that it doesn't feel too crowded up there.
Comment 12•6 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #8)
> :karlcow, this might be of high interest to you. So, I am needinfoing you in
> case you want to take an early try at it and put any immediate feedback so I
> can continue to iterate on it. Otherwise, expect it in Nightly hopefully
> this week.
Thanks Gabriel for the heads up. That's good news.
I will definitely play with this and comment back here when it lands on Nightly.
One question:
On the left side, there is the Responsive Menu, where it is possible to select a predefined set of values including the UA.
What is the story for the users when they have selected something on the left side, and they change the UA on the right side manually.
Flags: needinfo?(kdubost)
Comment 13•6 years ago
|
||
Comment on attachment 9007492 [details] [diff] [review]
1297431.patch [2.0]
Review of attachment 9007492 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/client/responsive.html/components/App.js
@@ +158,5 @@
> onRemoveCustomDevice(device) {
> + // If the custom device is currently selected on any of the viewports,
> + // remove the device association and reset all the ui state.
> + for (const viewport of this.props.viewports) {
> + if (viewport.device == device.name) {
Please use triple equals for strict checks. Just in case :)
::: devtools/client/responsive.html/components/Toolbar.js
@@ +74,5 @@
> + UserAgentInput({
> + onChangeUserAgent,
> + userAgent,
> + }),
> + dom.div({ className: "devtools-separator" }),
I'd prefer we don't use DOM nodes for decoration.
Not a blocker now, but maybe a task to note for a future refactoring to replace these separator nodes with either a CSS border or 1px wide background-image.
::: devtools/client/responsive.html/components/UserAgentInput.js
@@ +32,5 @@
> + this.onChange = this.onChange.bind(this);
> + this.onKeyUp = this.onKeyUp.bind(this);
> + }
> +
> + componentWillReceiveProps(nextProps) {
I believe `componentWillReceiveProps` is on track to be deprecated:
https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops
This isn't a blocker now, but it will need to be refactored when React gets eventually updated.
Recommendations for refactoring include using fully controlled components:
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#preferred-solutions
@@ +49,5 @@
> + };
> + });
> + }
> +
> + onKeyUp({ target, keyCode }) {
This is neat. I never knew you could destructure like this in the method signature. Perhaps a short code comment will help avoid confusion for others and future us.
@@ +69,5 @@
> + id: "user-agent-input",
> + onChange: this.onChange,
> + onKeyUp: this.onKeyUp,
> + placeholder: getStr("responsive.customUserAgent"),
> + ref: this.inputRef,
`inputRef` doesn't seem to be used anywhere in this component.
::: devtools/client/responsive.html/index.css
@@ +110,5 @@
> + width: 120px;
> +}
> +
> +#user-agent-input:focus {
> + width: 900px;
This `900px` makes the input be excessively wide for narrower viewports, pushing all controls to the right out of view.
There is a CSS-only solution to use up all available space if you're willing to drop the `display: grid` on `#toolbar` for which I don't fully understand its role.
Here's an idea:
```
#toolbar {
display: flex; // instead of grid
}
#toolbar-center-controls {
flex: 1;
}
// And the following wholly replaces the CSS changes in this patch
// to highlight obsolete code if going with this suggestion.
#user-agent-label {
display: flex;
align-items: center;
margin-inline-start: 3px;
margin-inline-end: 3px;
width: 300px;
}
#user-agent-label:focus-within {
flex: 1;
}
#user-agent-input {
margin-inline-start: 3px;
width: 100%;
}
#user-agent-input[value=""] {
/* width: 120px; */
}
#user-agent-input:focus {
/* width: 900px; */
/* transition: width 0.25s ease; */
}
#user-agent-input,
#user-agent-input[value=""]:focus {
/* width: 300px; */
}
```
Attachment #9007492 -
Flags: review?(rcaliman) → review+
Comment 14•6 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40bdf96e9fd
Add a custom user agent input in responsive design mode. r=rcaliman
Comment 15•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•