Closed Bug 1158853 Opened 5 years ago Closed 5 years ago

Replace tile titles with base domains and move them on top of the tile image

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: emtwo, Assigned: emtwo)

References

(Blocks 1 open bug)

Details

(Whiteboard: .?)

Attachments

(5 files, 2 obsolete files)

Attached image suggested.png
Aaron, I've made the changes to include the title/url as part of the tile here.

History tiles display the domain while directory + suggested tiles have the ability to provide a custom title. Just confirming this is what we wanted and that the suggested tile's text 'suggested for <xyz> visitors' should no longer be selectable.
Attachment #8599551 - Flags: ui-review?(athornburgh)
Attached image overlay.png (obsolete) —
I specifically wanted to ask about the overlay as seen here. Is the 'suggested' tag meant to be clickable? Or is it the entire title bar on the tile that is now clickable?

Should the overlay be covering the title bar as it is in this screenshot? or be above it?
Attachment #8599553 - Flags: ui-review?(athornburgh)
Attached patch WIP (obsolete) — Splinter Review
Clicking the "suggested" label will launch the black overlay – as it currently exists – on top of the screenshot. The overlay should NOT cover the title bar.

Clicking anywhere else in the title bar would launch a New Tab with the detestation page loaded.
Also, you are correct in Comment #1 regarding tile titles... main-level domain only for History (nfl.com) and client suggested tiles, or a custom title for Mozilla Directory Tiles.
Whiteboard: .?
Attachment #8599551 - Flags: ui-review?(athornburgh)
Attached image overlay.png
Attachment #8599553 - Attachment is obsolete: true
Attachment #8599553 - Flags: ui-review?(athornburgh)
Blocks: 1140185
Iteration: --- → 40.3 - 11 May
Blocks: 1160372
Comment on attachment 8599885 [details] [diff] [review]
v1: Replace history tile titles with base domains and include titles as part of tiles

Review of attachment 8599885 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/sites.js
@@ +121,5 @@
>     */
>    _render: function Site_render() {
>      let enhanced = gAllPages.enhanced && DirectoryLinksProvider.getEnhancedLink(this.link);
>      let url = this.url;
> +    let title = (enhanced && enhanced.title ? enhanced.title : (this.link.type == "history" ? this.link.baseDomain : this.title || this.url));

`this.title || this.url` at the end shouldn't be necessary, since you changed the title getter to return the same thing, right?  Only this.title should be OK.

And could you please write this like:

let title = enhanced && enhanced.title ? enhanced.title :
            this.link.type == "history" ? this.link.baseDomain :
            this.title;
Attachment #8599885 - Flags: review?(adw) → review+
No longer blocks: 1160372
No longer blocks: 1150228
Depends on: 1150228
https://hg.mozilla.org/mozilla-central/rev/3a0579ae7667
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Points: --- → 5
Depends on: 1167805
Blocks: 1168521
verified with nightly 2015-05-24

1) visited air.mozilla.org
2) saw tile on new tab page with "air.mozilla.org" instead of "Air Mozilla | Mozilla, in Video"
3) saw tile edges contain the title
Status: RESOLVED → VERIFIED
Attached image verify screenshot
Confirming the fix using latest Aurora, build ID: 20150528004000 on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
You need to log in before you can comment on or make changes to this bug.