Skip to content
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

Merged
merged 67 commits into from
Apr 13, 2022
Merged

Add EuiTimeline #5730

merged 67 commits into from
Apr 13, 2022

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Mar 17, 2022

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 icon

Screenshot 2022-03-23 at 14 49 28

EuiAvatar

Added new color option "subdued" to EuiAvatar

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

miukimiu and others added 9 commits April 7, 2022 11:21
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>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@miukimiu
Copy link
Contributor Author

miukimiu commented Apr 7, 2022

@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 "top". Now it works no matter which alignment is passed.

alignments

I also added a recommendation for the icon size in 28496cb.

@cchaos
Copy link
Contributor

cchaos commented Apr 7, 2022

🙌 Nice catch!!! I'll do a final review later today

Copy link
Contributor

@cchaos cchaos left a 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} />
Copy link
Contributor

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:

  1. Require an event aria-label (or something) like we require for EuiButtonIcon's which in this case would just pass down as the avatar name.
  2. 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.
Screen Shot 2022-04-11 at 15 50 34 PM

And if it's purely decorative, then this extra tooltip is just more noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cchaos, this was fixed in 2c9c667. I added an iconAriaLabel prop that only works when the icon is passed as an IconType. The prop is not required. If not passed we assume the icon is decorative.

Copy link
Contributor

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.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

Copy link
Contributor

@cchaos cchaos left a 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.

src-docs/src/views/timeline/timeline_example.js Outdated Show resolved Hide resolved
Copy link
Contributor

@thompsongl thompsongl left a 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>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5730/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiSteps] Add more flexibility/customizability
8 participants