-
-
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 Color Swap Red #3730
Design System Audit Color Swap Red #3730
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.
|
Available: Fri 11/18 |
Availability: 9 - 11 PM 17NOV22, 18NOV22 |
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.
Good job @esantiano! You successfully changed all of the instances that had a hard coded red color with its equivalent scss variable. The changes also didn't affect the website.
One thing I want to note is that I noticed some files had other hard-coded colors that don't seem to exist in the _colors.scss file. A couple of examples are linked below. It's not within the scope of this issue but it's something I thought I should bring up to see if they should be replaced with a variable too.
Line 147 in 87c6ab8
style="margin: 0; border: 2px solid #336699; box-sizing: border-box"> |
color: #000000; |
ETA:18/11/2022 |
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.
Good work @esantiano ! You made suitable changes in the files as per the comment.
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.
Hi @esantiano - Looks great!
Appears that all references to hard-coded #fa114f
(outside of the excluded files) have been changed to $color-red
per the issue. As it says in the linked issue #2112, it also looks like references to the other reds (i.e. $color-firebrick
, etc.) are to the color variable name and not the hard-coded color and that all are being used.
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.
Hi @esantiano ,
Great job replacing the instances of #fa114f
with $color-red
! 👍
However, I wonder if the $color-salmon
variable from _colors.scss and the .color-salmon
class from _color-styles.scss should be removed since they seem to be unused throughout the code base.
UPDATE
rgb(255, 173, 173)
matches the$color-salmon
#ffadad
and is used for the.status-Completed
class'sbackground-color
property found in _home.scss , _projects-pages.scss, and _projects.scss. Perhaps these 3 instances ofrgb(255, 173, 173)
should be changed to use the$color-salmon
variable?rgb(250, 17, 79)
matches$color-red:
#fa114f
and is used 4 times in the _about.scss file. This could also probably be swapped.
Hi @Adastros good job catching those instances, its my understanding that these issues will be addressed in the future related to issue #2114. |
|
cacc28f
Hi @MattPereira
I think this might be addressed with a new issue as well.
After looking at the about.scss I found that the change from |
Hey @esantiano ,
|
ceccfd7
to
78bd70d
Compare
Hey @MattPereira Just changed these instances - the changes were applied to the mobile site and haven't affected any of the colors used. |
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.
@esantiano good work replacing all remaining hard-coded instances of "reds" with the corresponding color variables ! 👍
Fixes #2112
What changes did you make and why did you make them ?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
No visual changes to the website, color variable swap should allow the website to remain the same.
Visuals after changes are applied
No visual changes to the website, color variable swap should allow the website to remain the same.