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

[EuiCommentList] Convert to Emotion, allow custom children and new event props #6030

Merged
merged 15 commits into from
Jul 19, 2022

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Jul 6, 2022

Summary

This PR adds new props and other enhancements to EuiCommentList/EuiComment. Part of the discussions that led to these enhancements can be found here: #5692.

Other enhancements are a result of our consumer's feedback on the usage of this component:

  1. There were incorrect usages of this component. For example to create life cycle timelines. This led to the creation of the EuiTimeline.
  2. Based on user/consumers feedback the mix of avatars/update icons was confusing.
  3. There was no way to call out an update (have a different background color). So in a very long list of comments if an update was important it was lost.
  4. Consumers were overriding styles to include custom components in a comment list, for example, to add a markdown editor.

This is the Figma file where you can find all the research and design ideas:
https://www.figma.com/file/OlPQKfamN8WzA7jT7dI16z/CommentList?node-id=0%3A1

Converted to Emotion

The EuiCommentList and EuiComment were updated to use EuiTimeline and EuiTimelineItem respectively. So this reduces a lot of the custom styles. At the end, there was only one component that required custom styles and it was converted to emotion: comment_event.styles.ts

Custom children

Now consumers can add custom components without the username and a panel appearing.

custom@2x

New props

New props eventIcon, eventIconAriaLabel, and eventColor:

event props@2x.

Also, a new gutterSize prop which is inherited from EuiTimeline was added.

Timeline icon

The timelineIcon is now only for avatars. To display an icon for an update/event consumers should be using the new eventIcon prop.

If no timelineIcon is provided, it will default to the username's initials. Before this update, the timelineIcon was defaulting to a user avatar. Using avatars with username's initials makes more sense for a comment list because it makes it easier to distinguish different users by initials/colors.

timeline@2x

EuiComment.actions now accepts ReactNode[]

Updated EuiComment.actions type to accept ReactNode[]

Screenshot 2022-07-11 at 15 47 26

New docs examples

Two new section were added to the docs:

Screenshot 2022-07-11 at 16 05 53

Screenshot 2022-07-11 at 15 49 56

Breaking changes

  • Changed EuiCommentEvent.username type from ReactNode to string The reason for that is that now the timeline icon defaults to an avatar with an initial username letter. So we need to make sure the username is a string to pass to the EuiAvatar.name.
  • Updated EuiCommentList and EuiComment to use EuiTimeline and EuiTimelineItem respectively. This change makes the EuiCommentList to be always required. It also improves the a11y by using better semantic elements (ol > li)
  • Removed EuiComment.type. It is no longer required to say what is the type of comment. The comment is now flexible enough and adapts to the props passed.

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

@elizabetdev elizabetdev changed the title [EuiCommentList] Convert to emotion, allow custom children and new event props [EuiCommentList] Convert to Emotion, allow custom children and new event props Jul 6, 2022
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev added this to the Elastic Stack 8.4 milestone Jul 11, 2022
@elizabetdev elizabetdev marked this pull request as ready for review July 11, 2022 15:50
@chandlerprall chandlerprall self-requested a review July 13, 2022 19:14
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Initial review for src-docs changes.


One suggested change doesn't fit the PR feedback format well, so I've opened a PR for that one - elizabetdev#30


Should eventColor=plain cause this drop shadow?

Screen Shot 2022-07-13 at 1 56 03 PM

src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_example.js Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_props.tsx Outdated Show resolved Hide resolved
src-docs/src/views/comment/comment_system.tsx Outdated Show resolved Hide resolved
…_flexible

[comment flexible example] remove 'any' type usage
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

Thanks, @chandlerprall. I went through your review and pushed 7e31abc and 2bc273f. Let me know if there is anything missing.

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

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

Successfully merging this pull request may close these issues.

3 participants