1334444 - [compact layout] Add "Your Top Sites" heading

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1334444 - [compact layout] Add "Your Top Sites" heading.

Review commit: https://reviewboard.mozilla.org/r/107758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107758/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8831098 [details]
Bug 1334444 - [compact layout] Add "Your Top Sites" heading.

https://reviewboard.mozilla.org/r/107758/#review108906

Shouldn't we also update the calculations for how many rows/columns fit as this header will take up vertical space inside the grid container? Maybe it would be easier to have it outside the grid container?

I was also under the impression Verdi wanted it to say "Top Sites" while there was no history (as they're not really the user's if they're just a default set...), and "Your Top Sites" when there was. Can we check this with him?

::: browser/themes/shared/newtab/newTab.inc.css:103
(Diff revision 1)
> +body.compact #newtab-grid {
> +  position: relative;
> +}
> +
> +body.compact #newtab-grid {
> +  padding-top: 1em;
> +}

Why 2 selectors?

::: browser/themes/shared/newtab/newTab.inc.css:112
(Diff revision 1)
> +body.compact #newtab-grid {
> +  padding-top: 1em;
> +}
> +
> +#topsites-heading {
> +  color: #7A7A7A;

Should this just be graytext?

::: browser/themes/shared/newtab/newTab.inc.css:116
(Diff revision 1)
> +  position: absolute;
> +  top: 0;

Can you elaborate on why the position changes are necessary (ideally in the extended (ie not 1st line) commit message)? Also, why have both margin: and position: absolute? Doesn't the margin not affect anything anymore?

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #2)
> Comment on attachment 8831098 [details]
> Bug 1334444 - [compact layout] Add "Your Top Sites" heading.
> 
> https://reviewboard.mozilla.org/r/107758/#review108906
> 
> Shouldn't we also update the calculations for how many rows/columns fit as
> this header will take up vertical space inside the grid container?

I tried to use position:absolute so that the heading doesn't affect that stuff, but I think I need to add a negative top margin on the grid to make that fully effective.

> I was also under the impression Verdi wanted it to say "Top Sites" while
> there was no history (as they're not really the user's if they're just a
> default set...), and "Your Top Sites" when there was. Can we check this with
> him?

I didn't understood him this way and it doesn't seem worthwhile to me, but we can ask him in our next meeting and file a followup.

> ::: browser/themes/shared/newtab/newTab.inc.css:103
> (Diff revision 1)
> > +body.compact #newtab-grid {
> > +  position: relative;
> > +}
> > +
> > +body.compact #newtab-grid {
> > +  padding-top: 1em;
> > +}
> 
> Why 2 selectors?

An oversight.

> ::: browser/themes/shared/newtab/newTab.inc.css:112
> (Diff revision 1)
> > +body.compact #newtab-grid {
> > +  padding-top: 1em;
> > +}
> > +
> > +#topsites-heading {
> > +  color: #7A7A7A;
> 
> Should this just be graytext?

No, this page doesn't use system colors.

> ::: browser/themes/shared/newtab/newTab.inc.css:116
> (Diff revision 1)
> > +  position: absolute;
> > +  top: 0;
> 
> Can you elaborate on why the position changes are necessary (ideally in the
> extended (ie not 1st line) commit message)? Also, why have both margin: and
> position: absolute? Doesn't the margin not affect anything anymore?

In case you're referring to the horizontal margin, that's to align the heading with the cells and doesn't have anything to do with position:absolute.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8831098 [details]
Bug 1334444 - [compact layout] Add "Your Top Sites" heading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107758/diff/1-2/

Comment 5

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #3)
> > I was also under the impression Verdi wanted it to say "Top Sites" while
> > there was no history (as they're not really the user's if they're just a
> > default set...), and "Your Top Sites" when there was. Can we check this with
> > him?
> 
> I didn't understood him this way and it doesn't seem worthwhile to me, but
> we can ask him in our next meeting and file a followup.

I've added this to the QX etherpad.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8831098 [details]
Bug 1334444 - [compact layout] Add "Your Top Sites" heading.

https://reviewboard.mozilla.org/r/107758/#review108936

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0aad844ced1
[compact layout] Add "Your Top Sites" heading. r=Gijs

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b0aad844ced1

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]: bug 1332318

[User impact if declined]: about:newtab may be harder to understand as we add more content

[Is this code covered by automated tests?]: the layout of the heading isn't covered specifically, but about:newtab is generally covered pretty well

[Has the fix been verified in Nightly?]: no

[Needs manual test from QE? If yes, steps to reproduce]: will be tested as part of the funnelcake QA

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: no

[Why is the change risky/not risky?]: it's limited to browser.newtabpage.compact = true

[String changes made/needed]: I hardcoded "Your Top Sites" for the uplift. I think the funnelcake will be limited to en-US, but even if not, having this single string be English for a limited audience seems acceptable to me.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8831987 [details] [diff] [review]
patch for uplift

about:newtab changes, aurora53+, beta52+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/594b19158136

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a42f620fc44a

Comment 13

[Andrei Vaida [:avaida]]

CC'ing Grover, just to make sure he's in the loop here.