Closed Bug 1208705 Opened 4 years ago Closed 4 years ago

Theme: Set accentColor on Android 5.0+

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: sebastian, Assigned: shubhamkjain, Mentored)

References

Details

(Whiteboard: [lang=xml][good first bug])

Attachments

(3 files, 1 obsolete file)

Currently we do not set an accentColor (for theming UI controls) in our theme. That's why checkboxes and headers have this cyan color in settings (See screenshot).
@Anthony: What's the best color to use here?
Flags: needinfo?(alam)
Mentor: s.kaspari
Whiteboard: [lang=xml][good first bug]
hello
I'm here for my first bug. Can you please help me through resolving bugs in firefox?
Assignee: nobody → shubhamkjain
Any input from Anthony so far ?
Hey Shubham!

I've been thinking about this more. 

It seems like the best color here would be our Action Orange (#E66000) which is what seems to be there.
Flags: needinfo?(alam) → needinfo?(shubhamjindal18)
Flags: needinfo?(shubhamjindal18) → needinfo?(shubhamkjain)
Attached patch color_patch.diff (obsolete) — Splinter Review
Set the accent color to action_orange. @aAnthony I actually made the same choice. In fact the screenshot named color/fennec_orange that i attached is actually action_orange. i had accidentally named it incorrectly.
Flags: needinfo?(shubhamkjain)
Attachment #8666860 - Flags: review?(s.kaspari)
Comment on attachment 8666860 [details] [diff] [review]
color_patch.diff

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

Nice. This is looking good!

The commit message is a bit misleading. This color does not relate to menus. It's used to tint UI controls in general (Android 5+). Maybe change it to something like: "Set colorAccent in v21+ theme.". And you can add the reviewer at the end of the message. So in this case the commit message could read something like this:

"Bug 1208705 - Set colorAccent in v21+ theme. r=sebastian"

Feel free to upload an updated patch. This does not require a new review.

I'll push the patch to try[1] for you. If all tests pass then you can add the "checkin-needed" [2] keyword to this bug in order to get your changes commited and pushed.

[1] https://wiki.mozilla.org/Build:TryServer 
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8666860 - Flags: review?(s.kaspari) → review+
cool, Thank you! 
I'll upload an updated patch shortly.
Attachment #8666860 - Attachment is obsolete: true
Attachment #8667202 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6720d34c74b4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1213223
Depends on: 1216503
You need to log in before you can comment on or make changes to this bug.