Closed Bug 1370459 Opened 2 years ago Closed 2 years ago

Polish the onboarding overlay UI to align with new visual spec

Categories

(Firefox :: General, enhancement, P1)

enhancement

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.
Flags: qe-verify+
QA Contact: jwilliams
Target Milestone: --- → Firefox 56
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Depends on: 1357046
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 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 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)
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?
will rebase based on bug 1357049's button style
Depends on: 1357049
mossop this is an overlay CSS only change so you can delegate the review to rexboy and fischer if you like.
Flags: needinfo?(dtownsend)
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 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+
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 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+
thanks!
Flags: needinfo?(dtownsend)
Keywords: checkin-needed
Attachment #8876608 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
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)
fixed Bug 1373731 and bug 1373612 as well
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Duplicate of this bug: 1373612
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
https://hg.mozilla.org/mozilla-central/rev/49d4a1aa666a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Depends on: 1375024
Depends on: 1378465
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.