Closed Bug 1134484 Opened 5 years ago Closed 5 years ago

Implement v1 color palette

Categories

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

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox40 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 1 obsolete file)

39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details
Find and replace all instances of the colors used!

This should probably be step 1 in completing meta bug 1127517.
Assignee: nobody → michael.l.comella
/r/4327 - Bug 1134484 - Add Fennec color palette to colors.xml. r=liuche
/r/4329 - Bug 1134484 - Replace all uses of @color/fennec_ui_orange. r=liuche

Pull down these commits:

hg pull review -r 42d07061b917fea4d52cbf29ae734ae7bd3591f8
Attachment #8569517 - Flags: review?(liuche)
Let's review and land this in pieces.

The commits in comment 1 are to demonstrate my workflow and make sure I'm not missing anything. I recommend reading the extended commit messages for my specific methodology. I intend to make a commit for each color I replace.
Keywords: leave-open
https://reviewboard.mozilla.org/r/4329/#review3587

I noticed that there's an instance of #FF9500 that you missed:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/search_colors.xml#9

Does Search activity use resources in a different way causing you not to update it?
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

https://reviewboard.mozilla.org/r/4325/#review3591
Attachment #8569517 - Flags: review?(liuche) → review+
https://reviewboard.mozilla.org/r/4329/#review3593

Assumed they did but apparently they don't - all better.
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

https://reviewboard.mozilla.org/r/4325/#review3619

Ship It!
Attachment #8569517 - Flags: review+
Attachment #8569517 - Flags: review?(liuche)
Attachment #8569517 - Flags: review+
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

/r/4327 - Bug 1134484 - Replace all uses of @color/action_orange with resource name. r=liuche
/r/4329 - Bug 1134484 - Replace all uses of @color/action_orange_pressed. r=liuche
/r/4481 - Bug 1134484 - Replace all uses of @color/link_blue. r=liuche
/r/4483 - Bug 1134484 - Replace uses of @color/placeholder_active_grey. r=liuche
/r/4485 - Bug 1134484 - Replace uses of @color/placeholder_grey. r=liuche

Pull down these commits:

hg pull review -r 69a2712f9fdaab5c5d57e9f1e99d8c0420cc30a7
https://reviewboard.mozilla.org/r/4485/#review3697

Ship It!

::: mobile/android/base/resources/values/colors.xml
(Diff revision 1)
>    <color name="text_color_tertiary">#9198A1</color>

One thing that is confusing is that there are now references to "inverse" or "tertiary," which have no point of reference now. This happens with the flavors of text_color_primary.

Perhaps there should be a comment referring to what uses "placeholder_grey" should be used (secondary text, etc).

Thoughts: Add a comment about the colors that are primary and secondary. Alternatively (or maybe in addition), file a bug to fix the inverse, tertiary, ect colors. This could be tricky because the "inverse" of text_color_primary is probably not the "inverse" of placeholder_grey" - in fact, inverse probably has no meaning outside the context of text.

We could also consider keeping text_color_primary/secondary, and just referring to placeholder grey.
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

https://reviewboard.mozilla.org/r/4325/#review3733

Ship It!
Attachment #8569517 - Flags: review?(liuche) → review+
We discussed via irc and irl:

(In reply to Chenxia Liu [:liuche] from comment #12)
> One thing that is confusing is that there are now references to "inverse" or
> "tertiary," which have no point of reference now. This happens with the
> flavors of text_color_primary.

The intent is to remove these eventually because we want to have a common set of names with which to talk to UX.

> Perhaps there should be a comment referring to what uses "placeholder_grey"
> should be used (secondary text, etc).

The names should (hopefully) be descriptive enough - we're in the process looking into renaming "placeholder grey" and "placeholder active grey" to something like "input placeholder grey" and "input active grey".
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

/r/4327 - Bug 1134484 - Replaces uses of @color/private_toolbar_grey. r=liuche
/r/4329 - Bug 1134484 - Replace uses of @color/text_and_tabs_tray_grey. r=liuche
/r/4481 - Bug 1134484 - Replace uses of @color/tabs_tray_grey_pressed. r=liuche
/r/4483 - Bug 1134484 - Replace uses of @color/toolbar_icon_grey. r=liuche
/r/4485 - Bug 1134484 - Replace uses of @color/tabs_tray_icon_grey. r=liuche
/r/6425 - Bug 1134484 - Replace uses of @color/disabled_grey. r=liuche
/r/6427 - Bug 1134484 - Replace uses of @color/toolbar_grey_pressed. r=liuche
/r/6429 - Bug 1134484 - Replace uses of @color/toolbar_menu_dark_grey. r=liuche
/r/6431 - Bug 1134484 - Replace uses of @color/toolbar_grey. r=liuche
/r/6433 - Bug 1134484 - Replace uses of @color/about_page_header_grey. r=liuche

Pull down these commits:

hg pull -r df66100448cd59cbf83fd4209baabe2c73cc104b https://reviewboard-hg.mozilla.org/gecko/
Attachment #8569517 - Flags: review+ → review?(liuche)
https://reviewboard.mozilla.org/r/4329/#review6329

::: mobile/android/base/resources/layout-large-land-v11/tabs_panel_sidebar.xml:16
(Diff revision 3)
> -              android:background="@color/background_tabs">
> +              android:background="@color/text_and_tabs_tray_grey">

I guess this file no longer exists. Sorry for the bitrot :(
https://reviewboard.mozilla.org/r/6433/#review6347

Ship It!

::: mobile/android/base/resources/values/colors.xml:29
(Diff revision 1)
>  

Might as well remove this newline - it's extraneous and looks confusing.

::: mobile/android/base/resources/layout-large-v11/new_tablet_tabs_counter.xml:13
(Diff revision 1)
> -                                         android:textColor="@color/text_color_primary_inverse"
> +                                         android:textColor="@color/about_page_header_grey"

Looks like this has been switched to @color/toolbar_grey in the current tree. Hm, by something that *I* reviewed. Okay.
Attachment #8569517 - Flags: review?(liuche) → review+
Comment on attachment 8569517 [details]
MozReview Request: bz://1134484/mcomella

https://reviewboard.mozilla.org/r/4325/#review6349

Ship It!
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Attachment #8569517 - Attachment is obsolete: true
Attachment #8619525 - Flags: review+
Attachment #8619526 - Flags: review+
Attachment #8619527 - Flags: review+
Attachment #8619528 - Flags: review+
Attachment #8619529 - Flags: review+
Attachment #8619530 - Flags: review+
Attachment #8619531 - Flags: review+
Attachment #8619532 - Flags: review+
Attachment #8619533 - Flags: review+
Attachment #8619534 - Flags: review+
You need to log in before you can comment on or make changes to this bug.