Closed
Bug 1222414
Opened 9 years ago
Closed 6 years ago
Convert hardcoded colors in add-on manager to CSS variables
Categories
(Toolkit :: Themes, defect)
Toolkit
Themes
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ntim, Assigned: concept_ray, Mentored)
References
Details
(Whiteboard: [good first bug][lang=css])
Attachments
(2 files, 7 obsolete files)
Many colors here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/extensions/extensions.inc.css could use CSS variables from https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css instead of some hardcoded colors.
Reporter | ||
Updated•9 years ago
|
Mentor: ntim.bugs
Whiteboard: [good first bug][lang=css]
Comment 1•9 years ago
|
||
Here is a new CSS file with most hardcoded colors replaced with color variables from common.inc.css.
Comment 2•9 years ago
|
||
Hi Tim, My name is Waqar and I am currently studying computer science degree. I would like to take this as my first bug as part of my opensource coursework and i was wondering if you could point me towards the right directions as in how to go about fixing this bug? Thank You
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Waqar Arshad from comment #2) > Hi Tim, > > My name is Waqar and I am currently studying computer science degree. I > would like to take this as my first bug as part of my opensource coursework > and i was wondering if you could point me towards the right directions as in > how to go about fixing this bug? > > Thank You Hi Waqar, thank you for your interest in this bug, the first comment in this bug should a good description of what to do, further more, it seems that Dillon has already done most the work, except it needs to be under the form of a patch. To generate a patch, you'll need to set up your development environment first. http://codefirefox.com is a good site with video tutorials. Feel free to use the needinfo? flag if you need help.
Comment 4•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #3) > (In reply to Waqar Arshad from comment #2) > > Hi Tim, > > > > My name is Waqar and I am currently studying computer science degree. I > > would like to take this as my first bug as part of my opensource coursework > > and i was wondering if you could point me towards the right directions as in > > how to go about fixing this bug? > > > > Thank You > Hi Waqar, thank you for your interest in this bug, the first comment in this > bug should a good description of what to do, further more, it seems that > Dillon has already done most the work, except it needs to be under the form > of a patch. > > To generate a patch, you'll need to set up your development environment > first. http://codefirefox.com is a good site with video tutorials. > > Feel free to use the needinfo? flag if you need help. Hi Tim, Could you please explain to me how i can run and check what effect the css file has and whether the changes that have been made are correct?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Waqar Arshad from comment #4) > Could you please explain to me how i can run and check what effect the css > file has and whether the changes that have been made are correct? You should verify that about:addons doesn't change visually.
Flags: needinfo?(ntim.bugs)
Comment 6•9 years ago
|
||
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8690245 [details] [diff] [review] The extensions css file now uses color variables in the linked css file Review of attachment 8690245 [details] [diff] [review]: ----------------------------------------------------------------- You should choose the variable appropriate to the context. ::: toolkit/themes/shared/extensions/extensions.inc.css @@ +156,5 @@ > padding: 2px 8px; > margin: 6px 0; > -moz-margin-start: 6px; > border-radius: 100%; > + color: var(--in-content-selected-text); This should stay as #fff @@ +220,5 @@ > > .view-header { > margin: 0; > -moz-margin-end: 48px; > + border-bottom: 1px solid var(--in-content-category-text); This is a border, so it should probably use the box-border-color variable @@ +227,5 @@ > #header-utils-btn { > height: 30px; > line-height: 20px; > + border-color: var(--in-content-category-text); > + background-color: var(--in-content-page-background); This is a button, so it should use box-background @@ +738,5 @@ > font-weight: bold; > } > > #detail-contrib-btn { > + color: var(--in-content-selected-text); This should stay as #fff @@ +743,2 @@ > text-shadow: none; > + border: 1px solid var(--in-content-link-color); This should use 1px solid transparent @@ +745,2 @@ > list-style-image: url("chrome://mozapps/skin/extensions/heart.png"); > + background-color: var(--in-content-link-color); This should be using in-content-primary-button-background as it's a primary button @@ +923,5 @@ > } > > .download-progress .status { > + color: var(--in-content-text-color); > + text-shadow: var(--in-content-selected-text) 0 0 2px; Please keep this as #fff @@ +970,5 @@ > -moz-appearance: none; > background: transparent; > border: none; > box-shadow: none; > + color: var(--in-content-primary-button-background); This should be --in-content-link-color
Comment 8•9 years ago
|
||
Hi, I want to fix this bug. This is my first bug. Can you guide me please
Comment 10•9 years ago
|
||
Hi, Can I have this bug assigned to me? Thanks
Comment 11•9 years ago
|
||
Hi Tim, Find my patch attached. Kind regards, Seema Shad
Attachment #8696945 -
Flags: review?(ntim.bugs)
Comment 12•9 years ago
|
||
Please find my attachment. I have followed your latest guidance. Forgot to publish it last night as I was waiting to get the bug assigned. Thanks
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8696945 [details] [diff] [review] bug1222414.patch Review of attachment 8696945 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/extensions/extensions.inc.css @@ +51,5 @@ > } > > .global-warning .warning-icon { > + background-color: var(--in-content-selected-text); > + box-shadow: 0 0 2px 5px var(--in-content-selected-text); Please keep these 2 property values as #fff. @@ +227,5 @@ > > #header-utils-btn { > height: 30px; > line-height: 20px; > + border-color: var(--in-content-category-text); This should be var(--in-content-box-border-color) @@ +744,2 @@ > text-shadow: none; > + border: 1px solid transparent var(--in-content-link-color); You should remove var(--in-content-link-color) since the border-color is already set to transparent. @@ +752,5 @@ > } > > #detail-contrib-btn:not(:active):hover { > + border-color: var(--in-content-primary-button-background-hover); > + background-color: var(--in-content-primary-button-background-hover); You can omit the border-color here, since transparent already takes care of duplicating the border-color from the background. @@ +757,5 @@ > } > > #detail-contrib-btn:active:hover { > + background-color: var(--in-content-primary-button-background-active); > + border-color: var(--in-content-primary-button-background-active); Same here @@ +853,5 @@ > margin: 2px 4px; > } > > .download-progress[mode="undetermined"] { > + border-color: var(--in-content-category-border-focus); I would use var(--in-content-border-focus) in this context
Attachment #8696945 -
Flags: review?(ntim.bugs) → feedback+
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Caius Sabau from comment #12) > Created attachment 8696950 [details] [diff] [review] > bug1222414.diff > > Please find my attachment. I have followed your latest guidance. Forgot to > publish it last night as I was waiting to get the bug assigned. > > Thanks See the comments I've made on Seema's patch. Also, Seema is the first person to request review from me, so I'll assign the bug to him. The other patches slipped off my radar (please use the needinfo box next time), please email me if you'd like me to suggest other bugs to work on.
Reporter | ||
Updated•9 years ago
|
Attachment #8696950 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8694793 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8690245 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → shads2
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 15•9 years ago
|
||
Although Seema is assigned to this bug, I just wanted to know if this version of my fix is correct. Thanks
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 16•9 years ago
|
||
(In reply to Caius Sabau from comment #15) > Created attachment 8697271 [details] [diff] [review] > bug1222414_v2.diff > > Although Seema is assigned to this bug, I just wanted to know if this > version of my fix is correct. > > Thanks Can you merge that patch with your previous patch ?
Flags: needinfo?(ntim.bugs)
Comment 17•9 years ago
|
||
Sorry, it makes sense. I should have just updated the patch. I don't know how to merge them now, unless I start all over again? Is there another way?
Reporter | ||
Comment 18•9 years ago
|
||
(In reply to Caius Sabau from comment #17) > Sorry, it makes sense. I should have just updated the patch. I don't know > how to merge them now, unless I start all over again? > Is there another way? Do `hg qpop` until only your first patch is applied. Then do: hg qfold name-of-your-second-patch
Comment 19•9 years ago
|
||
Awesome, think I made it. Can you please have a look at this patch now? Thank you!
Comment 21•9 years ago
|
||
Tim, I would like to know what do you think about my patch. Can you review it please? Thanks
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 22•8 years ago
|
||
Seems like Seema is no longer working on this bug.
Assignee: shads2 → sabau_caius
Flags: needinfo?(ntim.bugs)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(shads2)
Reporter | ||
Comment 23•8 years ago
|
||
Comment on attachment 8697318 [details] [diff] [review] bug1222414.diff Review of attachment 8697318 [details] [diff] [review]: ----------------------------------------------------------------- Almost there, thanks for the patch ! Also, can you change the commit message to : Bug 1222414 - Convert hardcoded colors in add-on manager to CSS variables. r=dao,ntim ::: toolkit/themes/shared/extensions/extensions.inc.css @@ +48,5 @@ > } > > .global-warning .warning-icon { > background-color: #fff; > + box-shadow: 0 0 2px 5px #fff; Please restore the indentation here. @@ +858,5 @@ > margin: 2px 4px; > } > > .download-progress[mode="undetermined"] { > + border-color:var(--in-content-border-focus); Please add a space after ":" @@ +866,5 @@ > .download-progress[complete] .end-cap, > .download-progress[mode="undetermined"] .end-cap, > .download-progress .progress .progress-bar { > -moz-appearance: none; > + background-color: ar(--in-content-border-focus); Should be var not ar
Attachment #8697318 -
Flags: feedback+
Reporter | ||
Updated•8 years ago
|
Attachment #8697271 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8696945 -
Attachment is obsolete: true
Comment 24•7 years ago
|
||
Hi, please let me know if I can work on this bug?
Reporter | ||
Comment 25•7 years ago
|
||
(In reply to Pooja Arora from comment #24) > Hi, please let me know if I can work on this bug? Assigned it to you, you can look at the current patch and its review feedback for guidance.
Assignee: sabau_caius → poojaarora014
Comment 26•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #25) > (In reply to Pooja Arora from comment #24) > > Hi, please let me know if I can work on this bug? > > Assigned it to you, you can look at the current patch and its review > feedback for guidance. Sure. Thank you!
Comment 27•7 years ago
|
||
Hiya, I'm interested in developing this bug. Can I be assigned to it please? Thank you :)
Comment 28•6 years ago
|
||
Hey Tim, it looks like most, if not all, of this was done in bug 1169679. Should we dupe it there?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 29•6 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #28) > Hey Tim, it looks like most, if not all, of this was done in bug 1169679. > Should we dupe it there? I don't think bug 1169679 did anything related to this bug. However, bug 1388761 did take care parts of this bug. There's still some left: https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/toolkit/themes/shared/extensions/extensions.inc.css#118,184,189,266,300,374,739,742,748,757,975,995-1010,1036
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 30•6 years ago
|
||
Hello, I'd like to work on this as my good first bug!
Assignee | ||
Comment 31•6 years ago
|
||
Patch for extensions.inc.css that adds color variables
Attachment #8957381 -
Flags: review+
Reporter | ||
Updated•6 years ago
|
Attachment #8957381 -
Flags: review+ → review?(ntim.bugs)
Reporter | ||
Updated•6 years ago
|
Attachment #8697318 -
Attachment is obsolete: true
Reporter | ||
Comment 32•6 years ago
|
||
Comment on attachment 8957381 [details] [diff] [review] Fix for bug 1222414 Review of attachment 8957381 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly fine, thanks for looking into this. I have a few comments, can you please address them so we can land this patch? ::: toolkit/themes/shared/extensions/extensions.inc.css @@ +181,4 @@ > } > > .category-badge { > + background-color: var(--in-content-page-color); Please leave this as #55D4FF :) The .category-badge uses special colors unique the add-on manager. @@ +185,5 @@ > padding: 2px 8px; > margin: 6px 0; > margin-inline-start: 6px; > border-radius: 100%; > + color: var(--in-content-selected-text); Please leave this as #FFF for the same reason. @@ +736,4 @@ > > #detail-contributions { > border-radius: 2px; > + border: 1px solid var(--in-content-box-border-colour); I think you mean border-color not border-colour :) @@ +1033,4 @@ > > .addon[active="true"] .experiment-bullet, > #detail-view[active="true"] #detail-experiment-bullet { > + fill: var(--in-content-box-background-active); Could you leave this as `fill: rgb(106, 201, 20)` ?
Attachment #8957381 -
Flags: review?(ntim.bugs) → review+
Reporter | ||
Updated•6 years ago
|
Attachment #8957381 -
Flags: review+ → feedback+
Reporter | ||
Updated•6 years ago
|
Assignee: poojaarora014 → concept_ray
Assignee | ||
Comment 33•6 years ago
|
||
Fix for bug 1222414 with suggested changes
Attachment #8957381 -
Attachment is obsolete: true
Attachment #8957568 -
Flags: review+
Reporter | ||
Comment 34•6 years ago
|
||
Comment on attachment 8957568 [details] [diff] [review] 1222414.patch Review of attachment 8957568 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Updated•6 years ago
|
Attachment #8957568 -
Flags: review+
Comment 35•6 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e620f50bbdcf Convert hardcoded colors in add-on manager to CSS variables. r=jaws, ntim
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e620f50bbdcf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter | ||
Comment 37•6 years ago
|
||
Congrats Raymond! Your patch is now in the latest Nightly :)
Assignee | ||
Comment 38•6 years ago
|
||
Awesome, thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•