Follow-up: replace about:home sync "links" (actually buttons) with links.

RESOLVED FIXED in Firefox 10

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

unspecified
Firefox 10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [verified in services])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Currently sync links are actually buttons, to conform with browser.js click listener code. Replace buttons with actual links and propagate to browser.js.
(Assignee)

Comment 1

6 years ago
Created attachment 570074 [details] [diff] [review]
v1: replaced buttons with links

verified visually through UI and ran tests from bug 675821.
Attachment #570074 - Flags: review?(philipp)
Comment on attachment 570074 [details] [diff] [review]
v1: replaced buttons with links

Looks ok to me. Over to Gavin!
Attachment #570074 - Flags: review?(philipp) → review?(gavin.sharp)
Comment on attachment 570074 [details] [diff] [review]
v1: replaced buttons with links

Do you really need to override the default <a> styling at all? I could see perhaps the color change being desirable, but the :hover text-decoration and cursor rules seem like they should be unnecessary. r=me with that addressed.
Attachment #570074 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 570189 [details] [diff] [review]
v2: removed underline css

Underline does seem unnecessary, but the cursor doesn't appear for me without explicitly adding it in the css, not sure why.
Attachment #570074 - Attachment is obsolete: true
Attachment #570189 - Flags: review?(gavin.sharp)
Comment on attachment 570189 [details] [diff] [review]
v2: removed underline css

Does the hover stuff work if you make the links have href="#"?
(Assignee)

Comment 6

6 years ago
No, href="#" still lacks hover cursor.
Created attachment 570399 [details] [diff] [review]
tweaked patch

This seems to work for me, does it seem OK to you?
Attachment #570399 - Flags: feedback?(liuche)
(Assignee)

Comment 8

6 years ago
Comment on attachment 570399 [details] [diff] [review]
tweaked patch

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

Yup, tried this out and it works for me as well. Adding patch that includes localization changes.
Attachment #570399 - Flags: feedback?(liuche) → feedback+
(Assignee)

Comment 9

6 years ago
Created attachment 570755 [details] [diff] [review]
v3: tweaked patch + compat recently landed localization changes
Attachment #570189 - Attachment is obsolete: true
Attachment #570189 - Flags: review?(gavin.sharp)
Attachment #570755 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #570399 - Attachment is obsolete: true
https://hg.mozilla.org/services/services-central/rev/9215e529ae7a
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
Blocks: 675821
No longer depends on: 675821

Updated

6 years ago
Assignee: nobody → liuche
Component: Firefox Sync: UI → General
Product: Mozilla Services → Firefox
QA Contact: sync-ui → general
verified with latest s-c build
Whiteboard: [fixed in services] → [verified in services]
https://hg.mozilla.org/mozilla-central/rev/9215e529ae7a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.