-
Notifications
You must be signed in to change notification settings - Fork 844
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 EuiTimeline #5730
Add EuiTimeline #5730
Conversation
b72f0b1
to
35702ac
Compare
35702ac
to
28e5452
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
d3562f3
to
b9a059e
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
@cchaos I went through your review. I also changed a bit the logic of the timeline icon lines in 181da63. The way the timeline was built before was assuming that larger components would always have the alignment I also added a recommendation for the icon size in 28496cb. |
🙌 Nice catch!!! I'll do a final review later 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.
Just some last-minute bugs found from the last round of updates. Also, I'd checkin with @1Copenut to see if he knows of any a11y guidance around timelines.
|
||
const iconRender = | ||
typeof icon === 'string' ? ( | ||
<EuiAvatar color="subdued" name={icon} iconType={icon} /> |
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 consumer's can now pass simply the iconType
as the icon
and it you're just passing that on as the name
of the EuiAvatar, I think we should do one of two things:
- Require an event
aria-label
(or something) like we require for EuiButtonIcon's which in this case would just pass down as the avatarname
. - See if an empty string is valid as an EuiAvatar
name
instead of passing down the icon name.
Just passing the icon name doesn't really present the user with useful information. They care more what the icon represents not what it is.
And if it's purely decorative, then this extra tooltip is just more noise.
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.
Thank you for the heads up @cchaos. I looked over the code before our team sync and didn't see anything that would be an a11y issue on first pass.
I have an idea about exploring this component as an ordered list with list items to provide contextual cues for screen readers. That idea is a pure enhancement, so I'll add it as a research ticket and reference this PR.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
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! Let's get it in the hands of a consumer. We can follow up with proper a11y since we're labelling it "Beta". But be sure to do so.
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.
Code changes LGTM!
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/ |
Summary
This PR closes #4492.
There is one implementation in Kibana that is using the EuiComment to create a vertical timeline like the one mentioned in #4492 - a vertical timeline to indicate the life cycle of data.
I could "fix" some props in EuiComment to make the component more open. But IMO the purpose of the component is not meant to be used to create a vertical timeline that indicates the life cycle of data. But to be used to add user/system comments and updates.
EuiTimeline
So this PR adds a EuiTimeline component where consumer just need to pass an array of EuiTimelineItem. Each EuiTimelineItem accepts:
icon
: main icon that appears on the left side.children
: event content. You can pass any node.verticalAlign
: vertical aligns the event with the iconEuiAvatar
Added new color option
"subdued"
to EuiAvatarChecklist