Closed Bug 1408331 Opened 2 years ago Closed 2 years ago

[dt-onboarding] Add footer to download Firefox Dev Edition

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

We are landing a first version of the devtools onboarding page in bug 1361080.

The mockups specify a footer to encourage users to download Firefox DevEdition. This bug is about adding the footer.

The current mockups are at https://docs.google.com/document/d/1ku6Jn12XdTFNno0phhLjlBD2H43k5NYswALYnA_UVBA/edit#heading=h.vihrfx1f6nm2 but the footer design might be slightly updated.
No longer blocks: dt-addon-uishim
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached image footer_screenshot.png
Victoria:

I added the footer (see screenshot) as follows:
- text color is white
- background is a linear gradient between blue-60 (#0060df) and blue-80 (#002275)
- dev-edition logo is 160px wide
- "Learn more" link points to https://www.mozilla.org/en-US/firefox/developer/ 

Let me know if that sounds ok!
Attachment #8922021 - Flags: feedback?(victoria)
Blocks: 1408339
This looks great! I think my only comment is the dev-edition logo seems (visually) a tiny bit too high compared to the text - I think just 3px too high :)

Thanks for determining all these things like the exact colors and sizes btw - I hope to have a better process for mockup hand-offs in the future. (Like using https://app.zeplin.io/ or something)
Comment on attachment 8922022 [details]
Bug 1408331 - add footer to about:devtools page;

https://reviewboard.mozilla.org/r/193006/#review199946

I have a few comment/questions before r+ this patch. 
Also, how can I test it ?

::: devtools/shim/aboutdevtools/aboutdevtools.css:73
(Diff revision 2)
> -.installpage-link::after {
> +.installpage-link::after,
> +.footer-link::after  {

could we add a specific css class for that (`.external` ) ?
Or a smarter rule (`a:not([href^="chrome"])`), or even plain `a` since we only have 2 of them, all externals.

::: devtools/shim/aboutdevtools/aboutdevtools.css:139
(Diff revision 2)
> +.footer {
> +  display: flex;
> +  align-items: center;
> +  justify-content: center;
> +  width: 100%;
> +  height: 300px;

is this a min-height we want ? I'm reluctant having fixed size

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:109
(Diff revision 2)
>            <p class="feature-desc">Find memory leaks and make your application zippy.</p>
>          </li>
>        </ul>
>      </div>
> +
> +    <div class="footer">

what about using `<footer>` ?

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:112
(Diff revision 2)
>      </div>
> +
> +    <div class="footer">
> +      <img class="dev-edition-logo"
> +           src="chrome://devtools-shim/content/aboutdevtools/images/dev-edition-logo.svg"
> +           alt=""/>

Nice to have put an alt attribute, maybe we could fill it (Firefox DevEdition logo) ?

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:115
(Diff revision 2)
> +      <img class="dev-edition-logo"
> +           src="chrome://devtools-shim/content/aboutdevtools/images/dev-edition-logo.svg"
> +           alt=""/>
> +      <div class="footer-message">
> +        <h1 class="footer-message-title">Firefox Developer Edition</h1>
> +        <div>

Maybe `<p>` tags would be more meaningful ?

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:119
(Diff revision 2)
> +        <a class="footer-link" href="https://www.mozilla.org/en-US/firefox/developer/" target="_blank">
> +          Learn more
> +        </a>

could this be put inside the previous container element with a `<br>` tag before it ? That way we can have consistent styling between the text and the link.

Or maybe we don't want that, and in that case, keep it as is :) .

::: devtools/shim/aboutdevtools/images/dev-edition-logo.svg:1
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 1024 1024">

I'm not sure, but shouldn't we add the license comment block at the top ?

::: devtools/shim/aboutdevtools/images/external-link.svg:1
(Diff revision 2)
>  <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

same question about license header
Attachment #8922022 - Flags: review?(nchevobbe) → review-
Side-note: In this page, we say to the user to open the devtools with the menu/kbd shortcuts, which is nice, but I wonder if we could get a step further and have a button for each "feature" in this page that would open the correct panel ? 
This is something that looks natural to me, the user parsing this page, seeing "debugger" and clicking on it to open the debugger.

I am really not sure how feasible this is though :)
Comment on attachment 8922022 [details]
Bug 1408331 - add footer to about:devtools page;

https://reviewboard.mozilla.org/r/193006/#review199946

Thanks for the review. To test it, simply go to about:devtools .
If you want to check that the footer is hidden if devtools are disabled, go to about:config and flip devtools.enabled to false. The page should update dynamically when you do so.

> could we add a specific css class for that (`.external` ) ?
> Or a smarter rule (`a:not([href^="chrome"])`), or even plain `a` since we only have 2 of them, all externals.

Done with .external, I prefer to keep rules explicit for now.

> is this a min-height we want ? I'm reluctant having fixed size

I think you are right. Otherwise if the user has a very big screen, they might see some blank space below the footer.

> could this be put inside the previous container element with a `<br>` tag before it ? That way we can have consistent styling between the text and the link.
> 
> Or maybe we don't want that, and in that case, keep it as is :) .

Not sure what this would achieve, but maybe I don't have the whole picture? 
Would this allow to simplify the CSS?

> I'm not sure, but shouldn't we add the license comment block at the top ?

done thanks!

> same question about license header

Done for this file + all existing SVGs in a second changeset.
So, this is looking fine, and I'm willing to r+ it.

I have the feeling we can make it better, though. The nice thing is that we have a grid already defined in features-list, and I wonder if we couldn't have it author the whole page.
So everything would go into the `<ul>` tag, like: 


```
<ul xmlns="http://www.w3.org/1999/xhtml" class="features-list">
    <img src="chrome://devtools-shim/content/aboutdevtools/images/otter.png" style="max-width: 100%;align-self: end;" />
    <div class="right-pane">
        <h1>Welcome to Firefox Developer Tools!</h1>
        <p class="message" id="welcome-message">You’ve successfully enabled DevTools! To get started, explore the Web Developer menu or open the tools with Cmd+Opt+I.</p>
    </div>
    <hr style="grid-column-start: 1;grid-column-end: -1;background-color: var(--grey-30);height: 1px;display: block;width: 100%;" class="" />

    <li class="feature">
        <img class="feature-icon" src="chrome://devtools-shim/content/aboutdevtools/images/feature-inspector.svg" alt="" />
        <h3 class="feature-name">Inspector</h3>
        <p class="feature-desc">Inspect and refine code to build pixel-perfect layouts.</p>
    </li>

    […]
</ul>
```

The logo would be one column large and the title would span across 2 columns.
That way, everything is neatly aligned.
Plus, it would make things easier in the bug where we plan to make this page work on narrow viewports.

Tell me what you think about this (you can also tell me that it's bad :) )
Comment on attachment 8923770 [details]
Bug 1408331 - add license header to about:devtools svg files;

https://reviewboard.mozilla.org/r/194918/#review199988

Thanks !
Attachment #8923770 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> So, this is looking fine, and I'm willing to r+ it.
> 
> I have the feeling we can make it better, though. The nice thing is that we
> have a grid already defined in features-list, and I wonder if we couldn't
> have it author the whole page.
> So everything would go into the `<ul>` tag, like: 
> 
> 
> ```
> <ul xmlns="http://www.w3.org/1999/xhtml" class="features-list">
>     <img src="chrome://devtools-shim/content/aboutdevtools/images/otter.png"
> style="max-width: 100%;align-self: end;" />
>     <div class="right-pane">
>         <h1>Welcome to Firefox Developer Tools!</h1>
>         <p class="message" id="welcome-message">You’ve successfully enabled
> DevTools! To get started, explore the Web Developer menu or open the tools
> with Cmd+Opt+I.</p>
>     </div>
>     <hr style="grid-column-start: 1;grid-column-end: -1;background-color:
> var(--grey-30);height: 1px;display: block;width: 100%;" class="" />
> 
>     <li class="feature">
>         <img class="feature-icon"
> src="chrome://devtools-shim/content/aboutdevtools/images/feature-inspector.
> svg" alt="" />
>         <h3 class="feature-name">Inspector</h3>
>         <p class="feature-desc">Inspect and refine code to build
> pixel-perfect layouts.</p>
>     </li>
> 
>     […]
> </ul>
> ```
> 
> The logo would be one column large and the title would span across 2 columns.
> That way, everything is neatly aligned.
> Plus, it would make things easier in the bug where we plan to make this page
> work on narrow viewports.
> 
> Tell me what you think about this (you can also tell me that it's bad :) )

Not sure about this. This means using non-list elements inside our <UL>? If we want to keep the list semantics we would have to define two grids here I guess.

Could this be handled in a follow up?
> Not sure about this. This means using non-list elements inside our <UL>? If we want to keep the list semantics we would have to define two grids here I guess.

Yes, you're right, until sub-grid is a thing.

> Could this be handled in a follow up?

Sure
Comment on attachment 8922022 [details]
Bug 1408331 - add footer to about:devtools page;

https://reviewboard.mozilla.org/r/193006/#review200054
Attachment #8922022 - Flags: review?(nchevobbe) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e25cb414c2f2
add footer to about:devtools page;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/21e6718ad041
add license header to about:devtools svg files;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/e25cb414c2f2
https://hg.mozilla.org/mozilla-central/rev/21e6718ad041
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8922021 [details]
footer_screenshot.png

Clearing the feedback flag
Attachment #8922021 - Flags: feedback?(victoria) → feedback+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.