-
Notifications
You must be signed in to change notification settings - Fork 414
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
Add flair to snackbar #1313
Add flair to snackbar #1313
Conversation
Nice. I will check out the code more later but the picture looks good. We are using |
background-color: var(--color-primary); | ||
border-radius: 10px; | ||
box-shadow: var(--box-shadow-layer); | ||
color: #f0f0f0; |
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.
This should use a text color variable. Probably just --color-text
or whatever we call it.
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's a snackbar specific variable now.
padding: $spacing-vertical; | ||
position: fixed; | ||
top: $spacing-vertical; | ||
background-color: var(--color-primary); |
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.
You should create a variable for this --snack-bar-bg
--snack-bar-color
So we can change them for dark mode. Similar to how we do it with buttons:
https://github.com/lbryio/lbry-app/blob/master/src/renderer/scss/_vars.scss#L93
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.
Those were added.
text-transform: uppercase; | ||
|
||
span { | ||
color: #f0f0f0; |
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.
This should just be a color variable. It's just the same as the color you have above so maybe this isn't needed?
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.
True, this was not needed at all.
@seanyesmunt I updated the code as suggested in the comments. |
min-width: min-content; | ||
&:hover { | ||
text-decoration: underline; | ||
.message { |
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. The only thing I would do is change this to snack-bar__message
and move it to the top level. Then I think it's ok to merge.
PR changed per review. Also, |
Awesome! Just add a changelog commit and I will merge |
Changelog updated. |
Thanks @miikkatu! Have your contribution email, will follow up soon! |
This is for #1279.
Unlike modal dialogs in the redesign, this is with a dark background to make it pop a bit more. Functionality remains the same as with the original snackbar.
The prop-types package was added as a dependency.