Julian Descottes [:jdescottes] Assignee
	Description • 7 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 7 years ago

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!

	Victoria Wang [:victoria]
	Comment 3 • 7 years ago

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)

	Nicolas Chevobbe [:nchevobbe]
	Comment 6 • 7 years ago
mozreview-review

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

	Nicolas Chevobbe [:nchevobbe]
	Comment 9 • 7 years ago

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 :)

	Julian Descottes [:jdescottes] Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

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.

	Nicolas Chevobbe [:nchevobbe]
	Comment 11 • 7 years ago

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 :) )

	Nicolas Chevobbe [:nchevobbe]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8923770 [details]
Bug 1408331 - add license header to about:devtools svg files;

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

Thanks !

	Julian Descottes [:jdescottes] Assignee
	Comment 13 • 7 years ago

(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?

	Nicolas Chevobbe [:nchevobbe]
	Comment 14 • 7 years ago

> 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

	Nicolas Chevobbe [:nchevobbe]
	Comment 15 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 18 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e25cb414c2f2
https://hg.mozilla.org/mozilla-central/rev/21e6718ad041

	Victoria Wang [:victoria]
	Comment 20 • 7 years ago

Comment on attachment 8922021 [details]
footer_screenshot.png

Clearing the feedback flag