Closed
Bug 1370459
Opened 7 years ago
Closed 7 years ago
Polish the onboarding overlay UI to align with new visual spec
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: rexboy, Assigned: gasolin)
References
Details
(Whiteboard: [photon-onboarding])
Attachments
(1 file, 1 obsolete file)
We'll need to do a visual polish for the revised UX spec.
Hopefully this is going to be started when the visuals and assets (and maybe texts) are all locked down.
specs see bug 1354707.
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: jwilliams
Assignee | ||
Updated•7 years ago
|
Target Milestone: --- → Firefox 56
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Please ignore `Polyfill - Add private browsing and search tour to onboarding overlay.`, which will be replaced after `Bug 1357046 - Should add the Private Browsing tour in the onBoarding overlay` is landed.
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review152424
::: browser/extensions/onboarding/content/onboarding.css:65
(Diff revision 1)
> background: #f5f5f7;
> border: 1px solid rgba(9, 6, 13, 0.1); /* #09060D, 0.1 opacity */
> + border-radius: 3px;
> position: relative;
> - margin: 0 calc(50% - 600px);
> - top: calc(50% - 275px);
> + margin: 0 calc(50% - 480px);
> + top: calc(50% - 255px);
Please use
```
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
```
This way can deouple from the width and height.
::: browser/extensions/onboarding/content/onboarding.css:146
(Diff revision 1)
> }
>
> .onboarding-tour-description {
> grid-row: tour-page-start / tour-page-end;
> grid-column: tour-page-start / tour-content-start;
> - padding: 40px;
> + /*padding: 40px;*/
Why a comment out rule here?
Attachment #8876609 -
Flags: review?(fliu)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review153240
Per discussion I think we are still waiting for the latest visual assets to be confirmed.
Attachment #8876609 -
Flags: review?(rexboy)
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review153342
::: browser/extensions/onboarding/content/onboarding.css:66
(Diff revision 2)
> border: 1px solid rgba(9, 6, 13, 0.1); /* #09060D, 0.1 opacity */
> + border-radius: 3px;
> position: relative;
> - margin: 0 calc(50% - 600px);
> - top: calc(50% - 275px);
> + top: 50%;
> + offset-inline-start: 50%;
> + transform: translate(-50%, -50%);
What's the purpose for changing from margin to transform? It puts the overlay wrong when viewport height is less than 510px. If for some reason we have to use transform please consider the rule below inside media query together.
And I believe we can place the overlay right without using transform.
::: browser/extensions/onboarding/content/onboarding.css:114
(Diff revision 2)
>
> #onboarding-tour-list > li {
> list-style: none;
> height: 48px;
> border-inline-start: 6px solid transparent;
> + border-radius: 2px;
I saw the border has been removed in the spec.
::: browser/extensions/onboarding/content/onboarding.css:115
(Diff revision 2)
> #onboarding-tour-list > li {
> list-style: none;
> height: 48px;
> border-inline-start: 6px solid transparent;
> + border-radius: 2px;
> padding-inline-start: 66px;
and padding-inline-start being 49px
::: browser/extensions/onboarding/content/onboarding.css:116
(Diff revision 2)
> list-style: none;
> height: 48px;
> border-inline-start: 6px solid transparent;
> + border-radius: 2px;
> padding-inline-start: 66px;
> + margin-bottom: 9px;
and margin-inline-start being 10px.
::: browser/extensions/onboarding/content/onboarding.css:155
(Diff revision 2)
> + padding-inline-end: 28px;
> }
>
> .onboarding-tour-description > h1 {
> font-size: 36px;
> - margin: 40px 0 20px 0;
> + margin-top: 40px;
Let's use a smaller value to make it vertically centered like 25px
::: browser/extensions/onboarding/content/onboarding.css:181
(Diff revision 2)
> height: 100%;
> border: none;
> }
>
> .onboarding-tour-button > button {
> + position: absolute;
Do you still prefer using position: absolute button than just stretch page grid down to footer-end?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
mossop this is an overlay CSS only change so you can delegate the review to rexboy and fischer if you like.
Flags: needinfo?(dtownsend)
Assignee | ||
Comment 12•7 years ago
|
||
Here's how it looks like after the patch
http://g.recordit.co/bsnLkp4mbu.gif
Comment 13•7 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #12)
> Here's how it looks like after the patch
>
> http://g.recordit.co/bsnLkp4mbu.gif
This is inaccessible for me
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review154174
Attachment #8876609 -
Flags: review?(dtownsend) → review+
Reporter | ||
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review154296
Looks good to me. Thanks.
Attachment #8876609 -
Flags: review?(rexboy) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8876609 [details]
Bug 1370459 - Polish the onboarding overlay UI to align with new visual spec;
https://reviewboard.mozilla.org/r/147936/#review154320
Attachment #8876609 -
Flags: review?(fliu) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8876608 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fa6bbe236e0
Polish the onboarding overlay UI to align with new visual spec;r=Fischer,mossop,rexboy
Keywords: checkin-needed
Comment 20•7 years ago
|
||
Backed out for failing browser_parsable_css.js: icons_tour-complete.svg missing:
https://hg.mozilla.org/integration/autoland/rev/7e97fe54970f987fbc4a18422cd08013e1561a40
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5fa6bbe236e081e36b80264f6456de04db42af99&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107599308&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | missing resource://onboarding/img/icons_tour-complete.svg referenced from resource://onboarding/onboarding.css
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
fixed Bug 1373731 and bug 1373612 as well
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Comment 25•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/49d4a1aa666a
Polish the onboarding overlay UI to align with new visual spec;r=Fischer,mossop,rexboy
Keywords: checkin-needed
Comment 26•7 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•