Convert hardcoded colors in add-on manager to CSS variables

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: ntim, Assigned: concept_ray, Mentored)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

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

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Updated

4 years ago
Mentor: ntim.bugs
Whiteboard: [good first bug][lang=css]

Comment 1

4 years ago
Here is a new CSS file with most hardcoded colors replaced with color variables from common.inc.css.

Comment 2

4 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

4 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

4 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

4 years ago
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 5

4 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)
(Reporter)

Comment 7

3 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

3 years ago
Hi, I want to fix this bug. This is my first bug. Can you guide me please

Comment 9

3 years ago

Comment 10

3 years ago
Hi, 

Can I have this bug assigned to me?

Thanks

Comment 11

3 years ago
Posted patch bug1222414.patch (obsolete) — Splinter Review
Hi Tim,

Find my patch attached.

Kind regards,
Seema Shad
Attachment #8696945 - Flags: review?(ntim.bugs)

Comment 12

3 years ago
Posted patch bug1222414.diff (obsolete) — Splinter Review
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

3 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

3 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

3 years ago
Attachment #8696950 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8694793 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8690245 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Assignee: nobody → shads2
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 15

3 years ago
Posted patch bug1222414_v2.diff (obsolete) — Splinter Review
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

3 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

3 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

3 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

3 years ago
Posted patch bug1222414.diff (obsolete) — Splinter Review
Awesome, think I made it. Can you please have a look at this patch now?

Thank you!
(Reporter)

Comment 20

3 years ago
Seema, What's the status of this bug ?
Flags: needinfo?(shads2)

Comment 21

3 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

3 years ago
Seems like Seema is no longer working on this bug.
Assignee: shads2 → sabau_caius
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

3 years ago
Flags: needinfo?(shads2)
(Reporter)

Comment 23

3 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

3 years ago
Attachment #8697271 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8696945 - Attachment is obsolete: true

Comment 24

2 years ago
Hi, please let me know if I can work on this bug?
(Reporter)

Comment 25

2 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

2 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

2 years ago
Hiya, I'm interested in developing this bug. Can I be assigned to it please? Thank you :)
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

a year 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

a year ago
Hello, I'd like to work on this as my good first bug!
(Assignee)

Comment 31

a year ago
Posted patch Fix for bug 1222414 (obsolete) — Splinter Review
Patch for extensions.inc.css that adds color variables
Attachment #8957381 - Flags: review+
(Reporter)

Updated

a year ago
Attachment #8957381 - Flags: review+ → review?(ntim.bugs)
(Reporter)

Updated

a year ago
Attachment #8697318 - Attachment is obsolete: true
(Reporter)

Comment 32

a year 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

a year ago
Attachment #8957381 - Flags: review+ → feedback+
(Reporter)

Updated

a year ago
Assignee: poojaarora014 → concept_ray
(Assignee)

Comment 33

a year ago
Posted patch 1222414.patchSplinter Review
Fix for bug 1222414 with suggested changes
Attachment #8957381 - Attachment is obsolete: true
Attachment #8957568 - Flags: review+
(Reporter)

Comment 34

a year ago
Comment on attachment 8957568 [details] [diff] [review]
1222414.patch

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

Looks good, thanks!
Attachment #8957568 - Flags: review+ → review+

Comment 35

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e620f50bbdcf
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Reporter)

Comment 37

a year ago
Congrats Raymond! Your patch is now in the latest Nightly :)
(Assignee)

Comment 38

a year ago
Awesome, thanks!
(Reporter)

Updated

a year ago
Depends on: 1449012
You need to log in before you can comment on or make changes to this bug.