Polish the onboarding overlay UI to align with new visual spec

VERIFIED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
3 months ago
16 days ago

People

(Reporter: rexboy, Assigned: gasolin)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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

3 months ago
Flags: qe-verify+
QA Contact: jwilliams
(Assignee)

Updated

3 months ago
Target Milestone: --- → Firefox 56
(Assignee)

Updated

2 months ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Depends on: 1357046
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months 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

2 months 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

2 months 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

2 months 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?
(Assignee)

Comment 8

2 months ago
will rebase based on bug 1357049's button style
Depends on: 1357049
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 months 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

2 months ago
Here's how it looks like after the patch

http://g.recordit.co/bsnLkp4mbu.gif
(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

2 months 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

2 months 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

2 months 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)

Comment 17

2 months ago
thanks!
Flags: needinfo?(dtownsend)
Keywords: checkin-needed
(Assignee)

Updated

2 months ago
Attachment #8876608 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 19

2 months 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
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)
(Reporter)

Updated

2 months ago
Blocks: 1373612
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 months ago
fixed Bug 1373731 and bug 1373612 as well
Flags: needinfo?(gasolin)
Keywords: checkin-needed
(Reporter)

Updated

2 months ago
Duplicate of this bug: 1373612

Comment 25

2 months 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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49d4a1aa666a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Updated

2 months ago
Depends on: 1375024

Updated

2 months ago
Depends on: 1378465
(Assignee)

Updated

2 months ago
Blocks: 1379558
I have verified this fix on today's nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.