[compact layout] Add "Your Top Sites" heading

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks 1 bug)

Trunk
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(2 attachments)

No description provided.
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?
Attachment #8831098 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
(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 on attachment 8831098 [details]
Bug 1334444 - [compact layout] Add "Your Top Sites" heading.

https://reviewboard.mozilla.org/r/107758/#review108936
Attachment #8831098 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0aad844ced1
[compact layout] Add "Your Top Sites" heading. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/b0aad844ced1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Attachment #8831987 - Flags: approval-mozilla-beta?
Attachment #8831987 - Flags: approval-mozilla-aurora?
Comment on attachment 8831987 [details] [diff] [review]
patch for uplift

about:newtab changes, aurora53+, beta52+
Attachment #8831987 - Flags: approval-mozilla-beta?
Attachment #8831987 - Flags: approval-mozilla-beta+
Attachment #8831987 - Flags: approval-mozilla-aurora?
Attachment #8831987 - Flags: approval-mozilla-aurora+
Depends on: 1336072
Depends on: 1336434
CC'ing Grover, just to make sure he's in the loop here.
You need to log in before you can comment on or make changes to this bug.