-
Notifications
You must be signed in to change notification settings - Fork 47
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
Feature: Flash Message Styles and Animation #417
base: master
Are you sure you want to change the base?
Conversation
@CheetoMao when you get the chance, would love to get a code review. Thanks! |
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.
@HauwaAguillard work on the changes Ive mentioned below. I'll create an enhancement ticket to have the dismissible flash added. I'm going to work on some transitions.
- Make sure to re-zip the settings-templates.zip file
@include fade-out; | ||
} | ||
|
||
@mixin fade-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.
need to fix the transitions when the flash fades away and the page moves up so that its not jolting.
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'm going to work on this change.
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 would like to pair on this change when you are ready to work on it. Curious to see your workaround the jolting
* Add dismissable flash functionality * Fix spelling * Dismissible flash functionality
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.
Transitions still need work though.
</Table.Row> | ||
</Table.Head> | ||
<Table.HeadStacked> | ||
<Table.Data>Flash Props</Table.Data> |
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.
Markup for this should look like this:
<Table.HeadStacked>
<Table.Row>
<Table.Header>Flash Props</Table.Header>
</Table.Row>
</Table.HeadStacked>
.rev-FlashClose { | ||
background: none; | ||
border: none; | ||
color: $flash-color; |
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 probably get its own var. And will also need vars for hover
(added to :hover, :focus
) and active
(added to :active
)
$flash-close-color
$flash-close-color-hover
$flash-close-color-active
And don't forget to add them to the settings-template
medium-only: ((min-width: $medium) and (max-width: $large - 1)), | ||
large: (min-width: $large), | ||
large-down: ((max-width: $large - 1)), | ||
large-only: ((min-width: $large) and (max-width: $xlarge - 1)), | ||
xlarge: (min-width: $xlarge), | ||
xlarge-down: ((max-width: $xlarge - 1)), | ||
xlarge-down: ((max-width: $xlarge)), |
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.
change this back to xlarge-down: ((max-width: $xlarge - 1)),
@@ -14,18 +14,18 @@ $breakpoints: ( | |||
small: (min-width: $small), | |||
small-only: ((min-width: $small) and (max-width: $medium - 1)), | |||
medium: (min-width: $medium), | |||
medium-down: ((max-width: $medium - 1)), | |||
medium-down: ((max-width: $medium - 1 )), |
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.
remove empty space after the 1
scss/components/_Flash.scss
Outdated
border: $flash-border-size solid $flash-border-color; | ||
color: $flash-color; | ||
margin-bottom: $global-margin; | ||
padding: $global-padding-tiny 0; |
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.
Why isnt this using the $flash-padding
var?
&.rev-Flash--fade { | ||
@include fade-out--hide(); | ||
} | ||
&.rev-Flash--dismissible { |
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 would maybe even add a padding-right: $flash-dismissible-padding-right
so that the text doesnt overlap the close button. but I need to see what it looks like with the close button first. currently not seeing one on your branch.
if so, don't forget to add it to the settings-template
alert: ['rev-Flash--alert'], | ||
persistent: ['rev-Flash--persistent'], | ||
fade: ['rev-Flash--fade'], | ||
dismissible: ['rev-Flash--dismissible'], |
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.
needs to be added to the props table
} | ||
const BOOL_PROPS = [ | ||
...Object.keys(BOOL_PROPS_TO_CLASS_NAMES), | ||
'dismissible', |
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.
these two are props as well right? they need to be added to the props table as well then
PR connects to #415
Background
Initial styles and some animation for flash components.
Flash that fades is shown here: