Closed
Bug 1164307
Opened 9 years ago
Closed 8 years ago
Add @color/toolbar_divider_grey to the color palette & replace @color/divider_light
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mcomella, Assigned: raunaq.abhyankar, Mentored)
References
Details
(Whiteboard: [lang=java][lang=xml][good first bug][see comment 0])
Attachments
(1 file, 3 obsolete files)
15.33 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Attachment #8605654 -
Flags: review?(michael.l.comella)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → colemurray.cs
Reporter | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb16c14d68f7
Reporter | ||
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
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!
Reporter | ||
Comment 5•9 years ago
|
||
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
Reporter | ||
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
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)
Reporter | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d4f5482ef5
Reporter | ||
Comment 9•9 years ago
|
||
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+
Reporter | ||
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
Moved toolbar_divider_grey up in the colors.xml
Attachment #8609599 -
Attachment is obsolete: true
Attachment #8616409 -
Flags: review+
Reporter | ||
Comment 12•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(colemurray.cs)
Reporter | ||
Comment 13•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Whiteboard: [lang=java][lang=xml][good first bug] → [lang=java][lang=xml][good first bug][see comment 0]
Comment 14•9 years ago
|
||
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 ! :)
Reporter | ||
Comment 15•9 years ago
|
||
(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•8 years ago
|
||
Hi there, due to inactivity can I work on this bug?
Comment 17•8 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•8 years ago
|
||
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)
Reporter | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ea36ac2603
Reporter | ||
Comment 20•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → raunaq.abhyankar
Mentor: martyn.haigh+bugzilla
Reporter | ||
Updated•8 years ago
|
Attachment #8616409 -
Attachment is obsolete: true
Reporter | ||
Comment 21•8 years ago
|
||
Please add the "checkin-needed" keyword to get your patch landed – thanks!
Flags: needinfo?(raunaq.abhyankar)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(raunaq.abhyankar)
Keywords: checkin-needed
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b787719204aa
Keywords: checkin-needed
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b787719204aa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•