Adds StoreAlerts component #1627
Conversation
client/layout/store-alerts/index.js
Outdated
| this.totalAlerts = this.totalAlerts.bind( this ); | ||
| } | ||
|
|
||
| totalAlerts() { |
There was a problem hiding this comment.
I'm thinking eventually the array of alerts will be passed in as a prop, so we could probably update the logic in the component to be ready for that. We could create a defaultProps that passes in your dummy instance as the alerts prop. And you could then use this.props.alerts here
client/layout/store-alerts/index.js
Outdated
| const alert = dummy[ currentAlert ] ? dummy[ currentAlert ] : null; | ||
| const type = alertTypes[ alert.type ] ? alertTypes[ alert.type ] : null; | ||
| const className = classnames( 'woocommerce-store-alerts', { | ||
| 'is-alert-emergency': type && 'emergency' === alert.type, |
There was a problem hiding this comment.
Instead of checking for type here and having alert.type in each line, should we just assign alert.type to a const somewhere after line 103?
client/layout/store-alerts/index.js
Outdated
| role="status" | ||
| aria-live="polite" | ||
| > | ||
| { currentAlert + 1 } { __( 'of', 'wc-admin' ) } { this.totalAlerts() } |
There was a problem hiding this comment.
This might need to use interpolateComponents for a proper translation setup. Here is an example
client/layout/store-alerts/index.js
Outdated
| ] } | ||
| className={ className } | ||
| action={ | ||
| this.totalAlerts() > 1 && ( |
There was a problem hiding this comment.
If moved to props, we could assign the length of the alerts array to a const and use it throughout here.
const numberOfAlerts = this.props.alerts.length;
|
@timmyc thanks for the feedback! I made a few changes, hopefully it simplified things :) |
There was a problem hiding this comment.
Nice refactors @tiagonoronha - the props are looking great. One comment about putting a feature flag around this so it only shows in dev mode. Also I'm seeing a few glitches on mobile:
iOS / Mobile Safari buttons on next line
Not sure what the optimal positioning is on mobile ( cc @LevinMedia for any input here )
Android / Chrome
^ in both above, the alerts are also aligned with the left side of the screen and there is some margin on the right. Not sure if we want this full-width or with a bit of padding/margin on each side like we have for the date picker / filters.
| role="status" | ||
| aria-live="polite" | ||
| > | ||
| { interpolateComponents( { |
client/layout/index.js
Outdated
| @@ -66,6 +67,8 @@ class Layout extends Component { | |||
| <TransientNotices /> | |||
|
|
|||
| <div className="woocommerce-layout__primary" id="woocommerce-layout__primary"> | |||
| <StoreAlerts /> | |||
There was a problem hiding this comment.
Prior to merging this in, I think we should add a feature flag around this so it only shows up in Dev mode. #1500 has some examples on how that is done.
client/layout/store-alerts/index.js
Outdated
| } | ||
| > | ||
| <div className="woocommerce-store-alerts__message">{ alert.message }</div> | ||
| <Button isPrimary className="woocommerce-store-alerts__button" href={ alert.action.url }> |
There was a problem hiding this comment.
Can you change button to isDefault ?
There's some funky override going on in there somewhere, when I change it to isDefault the normal state button text is white, when it should be $core-grey-dark-500
There was a problem hiding this comment.
I'm also seeing the same issues on mobile that are noted above.
I'd like the left margin to match the rest of the content (it should be 16px on the smallest screens) and position the segmented button above the alert title?
|
Made a few changes :)
Also added the feature flag! |
|
@LevinMedia full context:
|
|
Thanks @tiagonoronha looking good! |
There was a problem hiding this comment.
This is looking great @tiagonoronha - I say we merge this and in the next sprint we can tackle how to add in real data to the priority notif area.
| @@ -67,7 +67,7 @@ class Layout extends Component { | |||
| <TransientNotices /> | |||
|
|
|||
| <div className="woocommerce-layout__primary" id="woocommerce-layout__primary"> | |||
| <StoreAlerts /> | |||
| { window.wcAdminFeatures[ 'store-alerts' ] && <StoreAlerts /> } | |||
Fixes #115
Basic component, still in development.