-
Notifications
You must be signed in to change notification settings - Fork 1
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
[MF] Creates global color, btn, font styling #84
Conversation
Visit the preview URL for this PR (updated for commit 808f00d): https://tcl-69-smart-shopping-list--pr84-mf-global-colors-btn-auc147p2.web.app (expires Wed, 10 Apr 2024 17:45:19 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 93172cc46147b7d365c2b1b8239b61e2efb07a80 |
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.
Looks good! I offer you a naming nit.
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.
Great work!
src/index.css
Outdated
--color-branding: #5a23b3; | ||
--btn-bg-color-confirm: var(--color-branding); | ||
--btn-bg-color-confirm-hover: #3f0f7f; | ||
--btn-bg-color-cancel: #cdcdcd; | ||
--btn-bg-color-cancel-hover: #989797; | ||
--btn-bg-color-delete: var(--color-error); | ||
--btn-bg-color-delete-hover: #a80000; |
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.
🔍 I like this, but I think we could bring even more clarity, theoretically. These variable names describe the function of the colors, but don't tell someone at a glance what the colors are. Is --btn-bg-color-confirm-hover
green? red? dandelion-ish?
In my time, I've seen CSS vars set up in 2 stages, with the first stage being the one that gives human-readable names to the colors, and the second stage assigning the function of the colors. I did something like this initially, although I didn't do the best job:
--color-red: hsl(0, 68%, 42%);
We know that this variable is a red, as opposed to a blue or a green. I think it makes sense to use that name when picking the background color for your button:
--btn-bg-color-delete: var(--color-red);
All of your other colors could follow the same pattern: first name the colors, then attach them to the elements they're used in.
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.
Ah ok I understand that now. That way when someone else looks at the line defining the --btn variable they will see a color named versus a hsl/hex/rgb. And I guess this also allows for that color to be reusable throughout the app as well?
Description
This code creates some CSS variables in the index.css file to be accessed through out the app. These variables are then applied to all HTML button elements throughout the app from the index.css page. The styles can be overwritten in particular instances at the level they need to be.
Related Issue
related to issue #14
closes issue #83
Acceptance Criteria
[x] Create CSS variables with brand colors including hovers
[x] Define at the index level how button inheritance style will look
[x] Confirm that the styles are applied in the app correctly
Type of Changes
Use one or more labels to help your team understand the nature of the change(s) you’re proposing. E.g.,
bug fix
orenhancement
are common ones.Updates
Before
Landing Page
Lists page
Manage-List Page
Dialog open
After
Landing page
Lists page
Manage-List Page
Dialog open
Testing Steps / QA Criteria
Nothing changed but styling. To test open app, sign in and walk through the functionality to observe and test all buttons to view the painting of the app with new css variables.