-
-
Notifications
You must be signed in to change notification settings - Fork 778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Design System Audit: Audit for hardcoded colors #4783
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Availability: Tuesday and Thursday morning. Sunday all day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey really nice job! This Issue is labelled as a small issue but it looks medium to me. Well done!
I found a few things that need changes and a suggestion.
- Just a suggestion: This color Design System Audit: Audit for hardcoded colors #2114 (comment) you said that is a type of white. It looks more like a green than a white. If you want, a little trick to name variables (since this is a tedious job) you can ask chat GPT. I asked for a name for that color and it suggested "Pale mint" which is pretty accurate haha. This would be an easy way to find more descriptive names for the variable names that contain numbers, like
color-darkblue-2
- This one: Design System Audit: Audit for hardcoded colors #2114 (comment) already has a variable called
color-darkblue
- In this one Design System Audit: Audit for hardcoded colors #2114 (comment) this color :
rgb(187, 255, 187)
in HEX is #BBFFBB which is hold by the variable calledcolor-lightgreen
- In this one Design System Audit: Audit for hardcoded colors #2114 (comment) that shorthand represents the HEX color #333333 that is hold by the variable called
color-black
- This one Design System Audit: Audit for hardcoded colors #2114 (comment) has a typo in the location. There is a score where there should be an underscore
_sass/components/-credit-items.scss
should be_sass/components/_credit-items.scss
- In this one Design System Audit: Audit for hardcoded colors #2114 (comment) the color in line 257 is hold by the variable
color-pink
- In this one Design System Audit: Audit for hardcoded colors #2114 (comment) you mention line 265 but you don't need to since you changed it for a variable.
- I didn't see any comment mentioning
_saas/components/_guides.scss
lines 52 and 225
Hi @93Belen, Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abdessittir The color names you labeled are all correct according to previous names that were used for the hardcodes referenced in the dependency documentations. So I approve what what done so far.
After browsing the keyword "color" in VScode, I was wondering if some of these other hardcodes should also be changed? Correct me if I'm wrong. I'm not actually sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! You addressed all my charges requests! Now you just need to update the PR description to match the changes you did.
Hi @93Belen thanks, I just updated my pull request description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Abdessittir for clarifying my question and adding more details in your description.
* Change hardcoded value to a variable * Change color values to color variables
* Change hardcoded value to a variable * Change color values to color variables
Fixes #2114
What changes did you make and why did you make them ?
background: #F6F6F6;
tobackground: $color-whitesmoke;
in file:_sass/components/_project-page.scss
color: #030D2D;
tocolor: $color-darkblue;
in file_sass/components/_donate-components.scss
_sass/components/_home.scss
,_sass/components/_privacy-policy.scss
,_sass/components/_projects-page.scss