Closed Bug 1334444 Opened 8 years ago Closed 8 years ago

[compact layout] Add "Your Top Sites" heading

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

      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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Attached patch patch for upliftSplinter Review
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.

Attachment

General

Created:
Updated:
Size: