Add @color/toolbar_divider_grey to the color palette & replace @color/divider_light

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: raunaq.abhyankar, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 46
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [lang=java][lang=xml][good first bug][see comment 0])

Attachments

(1 attachment, 3 obsolete attachments)

The color palette is located at the top of the mobile/android/base/resources/values/colors.xml file.

@color/toolbar_divider_grey should be #D7D9DB and replace all instances of @color/divider_light [1]. Also, @color/divider_light should be removed. I'd recommend using sed to do the find and replace.

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

Then, you'll need to create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

[1]: https://mxr.mozilla.org/mozilla-central/search?string=divider_light&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Depends on: 1148549
No longer depends on: 1159752
Created attachment 8605654 [details] [diff] [review]
I have added toolbar_divider_grey and replaced all instances of divider_light and removed divider_light.

Updated

3 years ago
Attachment #8605654 - Flags: review?(michael.l.comella)
Assignee: nobody → colemurray.cs
Comment on attachment 8605654 [details] [diff] [review]
I have added toolbar_divider_grey and replaced all instances of divider_light and removed divider_light.

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

Nice! Thanks for the patch, Cole!

I made a push to our try test servers (see above).

Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8605654 - Flags: review?(michael.l.comella) → review+
Michael,

It seems there was some conflict with some of the lower API's. I'm not quite sure how to interpret the errors. Assistance would be greatly appreciated!
B stands for "build" so in this run, we have some build failures.

If you click on one of the failed runs (i.e. click a "B"), you'll get a summary of the failure at the bottom of the page. If you look at the failure at the bottom of the page, you'll see:

build/mobile/android/base/resources/values/styles.xml:84: error: Error: No resource found that matches the given name (at 'android:divider' with value '@color/divider_light')

This fails across all builds, so I don't think it's related to lower level APIs. It looks like we may have missed a use of `@color/divider_light` (i.e. [1]).

Make sure you pull the latest copy of the code, rebase your changes, remove any additional references to @color/divider_light, and amend your commit (let me know if you need help with this). Note that pulling the latest source generally requires you to do a clobber build so, alternatively, you can search for `divider_light` to see if this line is already in your tree and amend there.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=divider_light&find=resources%2Fvalues%2Fstyles.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
By the way, you can use the "Need more information from" box below the comment field to give the user you'd like to talk to a notification - it's harder to lose track of. For example...
Flags: needinfo?(colemurray.cs)
Created attachment 8609599 [details] [diff] [review]
bug-1164307-fix.patch

I have corrected the skipped /divider_light references. Was able to build locally. Hopefully we see green!
Attachment #8605654 - Attachment is obsolete: true
Flags: needinfo?(colemurray.cs) → needinfo?(michael.l.comella)
Attachment #8609599 - Flags: review?(michael.l.comella)
Comment on attachment 8609599 [details] [diff] [review]
bug-1164307-fix.patch

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

Looks good - thanks, Cole!

r+ if fixing the nits below.

::: mobile/android/base/resources/values/colors.xml
@@ +90,5 @@
>    <!-- Link colors -->
>    <color name="text_color_link">#22629E</color>
>  
>    <!-- Divider colors -->
> +  <color name="toolbar_divider_grey">#D7D9DB</color>

Can you move this definition up the the color palette section near the top?
Attachment #8609599 - Flags: review?(michael.l.comella) → review+
By the way, you only need to set one notification flag - if you have a patch, you should probably use feedback or review. Otherwise, you can use needinfo.
Flags: needinfo?(michael.l.comella)
Created attachment 8616409 [details] [diff] [review]
bug-1164307-fix.patch

Moved toolbar_divider_grey up in the colors.xml
Attachment #8609599 - Attachment is obsolete: true
Attachment #8616409 - Flags: review+
I apologize - I forgot to mention you should add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run (which you have in comment 8).

Thanks, Cole!

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Flags: needinfo?(colemurray.cs)
Unassigning due to inactivity – unfortunately the patch no longer applies cleanly so this is still open as a good first bug if anyone is interested!
Assignee: colemurray.cs → nobody
Flags: needinfo?(colemurray.cs)
Whiteboard: [lang=java][lang=xml][good first bug] → [lang=java][lang=xml][good first bug][see comment 0]
Hello !
I would like to solve this bug. As it would be my first bug, I need a lil' guidance on how to do it. Thanks ! :)
(In reply to Srushtika Neelakantam from comment #14)
> Hello !
> I would like to solve this bug. As it would be my first bug, I need a lil'
> guidance on how to do it. Thanks ! :)

Welcome Srushtika! Comment 0 should give you the details on how to get started on this bug. If you have any more specific questions, let me know and I'll do my best to answer them.

Comment 16

3 years ago
Hi there, due to inactivity can I work on this bug?

Comment 17

3 years ago
(In reply to Sumeet (:gopianis) from comment #16)
> Hi there, due to inactivity can I work on this bug?

Sure, go for it.
(Assignee)

Comment 18

3 years ago
Created attachment 8705741 [details] [diff] [review]
bug-1164307-fix.patch

Added @color/toolbar_divider_grey to the color palette. Replaced all instances of @color/divider_light with @color/toolbar_divider_grey. Removed @color/divider_light from the color palette of Fennec.
Attachment #8705741 - Flags: review?(michael.l.comella)
Comment on attachment 8705741 [details] [diff] [review]
bug-1164307-fix.patch

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

This looks good to me. I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8705741 - Flags: review?(michael.l.comella) → review+
Assignee: nobody → raunaq.abhyankar
Mentor: martyn.haigh+bugzilla
Attachment #8616409 - Attachment is obsolete: true
Please add the "checkin-needed" keyword to get your patch landed – thanks!
Flags: needinfo?(raunaq.abhyankar)
(Assignee)

Updated

3 years ago
Flags: needinfo?(raunaq.abhyankar)
Keywords: checkin-needed

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b787719204aa
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.