-
Notifications
You must be signed in to change notification settings - Fork 0
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
Final styling changes #53
Conversation
Merge branch 'main' into pc-final-styling
Visit the preview URL for this PR (updated for commit 14d9446): https://tcl-73-smart-shopping-list--pr53-pc-final-styling-pizw4eak.web.app (expires Sat, 13 Apr 2024 00:15:37 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 99eed22fcca8bd9874e77f7b7f7d1eeb554a1666 |
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 Team work on this!
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 great to me!
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.
Your app looks beautiful and it functions really nicely. Good job!
.owner-message { | ||
background-color: hsla(000, 0.5%, 20%, 1); | ||
color: var(--color-text-dark); | ||
} |
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.
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.
We spent a long time trying to figure out how to match it to the background. We could not just use our background-dark css variable color because it has opacity, (that's why the color isn't exactly black but that charcoal color) so the background of that
tag that states the owner of the list would be clear which would show the list items behind it, which we didn't want. So we tried to match it as much as possible as much we could(we experimented with RGB and hex and hsla colors and landed with the one we chose as it looked pretty close on my screen. So even though isn't quite as bad on my screen, I do understand that it will vary from monitor to monitor and screen to screen. We are hoping this could just be something we address at a later time.
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.
Yeah, I kind of figured it was due to the opacity. It really doesn’t look bad, I just thought I’d double check.
Description
This PR included our final styling changes. We ran through the whole application, adjusting and making final changes to styling after many PR's were all merged.
Related Issue
Closes #14
Acceptance Criteria
Type of Changes
styling, enhancement, accessibility
Updates
Before
After
Testing Steps / QA Criteria
git pull
git checkout pc-final-styling
npm ci
npm run start