-
Notifications
You must be signed in to change notification settings - Fork 842
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
Updated EuiHeaderAlert #2506
Updated EuiHeaderAlert #2506
Conversation
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.
Left some comments, still need to review a bit.
48fc8c9
to
f14016c
Compare
@andreadelrio I lost track of this due to EAH and ElasticON, I'll re-check today! |
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.
LGTM, thanks for all changes! Looks like there is one small conflict to resolve, then this should be good to go.
<div className={classes} {...rest}> | ||
<EuiFlexGroup justifyContent="spaceBetween"> | ||
<EuiFlexItem> | ||
<div className="euiHeaderAlert__date">{date}</div> |
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.
Can we replace the div
with a time
tag?
<div className="euiHeaderAlert__date">{date}</div> | |
<time className="euiHeaderAlert__date">{date}</time> |
What might be a bit tougher but would love to see as well is to support automatically filling in a datetime
attribute. I'm not sure what EuiI18n
supports or not but instead of relying on Kibana (or other implementing devs) to pass in internationalized date strings, if EUI could accept an ISO8601 and then use that both in the datetime
attribute and then "translate" it for the human readable section that'd be awesome. Would look something like:
<time className="euiHeaderAlert__date" datatime={date}>
// NOTE: this is pseudo-code, I don't actually know how to make EuiI18n take a date
<EuiI18n value={date} />
</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.
@myasonik For the time being I'll just replace div
with time
. Sound good?
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.
It's ok if EuiI18n
can't handle dates...
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.
Since {date}
is a passed prop from the consuming app, EUI doesn't need to do any localization on it. It should be on the consumer to do that level. We only localize strings in EUI if they are hard-coded.
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.
LGTM! Thank you for getting those few a11y changes 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.
Checked out latest changes locally, looks good!
Summary
Updated
EuiHeaderAlert
and added a new section to the docs under Header/Alerts. Removed unneeded call toi18n
.Fixes #2502
Checklist
- [ ] Checked in dark mode- [ ] Checked in mobile- [ ] Checked in IE11 and Firefox- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes