[EuiComment] Add `"custom"` option to `EuiComment.type` and more enhancements #5692

elizabetdev · 2022-03-04T18:22:18Z

Summary

The idea of this PR was to add a docs example/demo of a comment system similar to the one from Security -> Cases.

But when I tried to build the demo I struggled to add a EuiMarkdownEditor inside a comment list. So I inspected the code of the Security -> Cases and noticed that they styled the comment list to make it work:

https://github.com/elastic/kibana/blob/513a08ea390156327a8fbeed4d6d6254315e02cd/x-pack/plugins/cases/public/components/user_actions/index.tsx#L43-L56

This research leads to introducing a new component called EuiTimeline #5730 and some enhancements in EuiCommentList and EuiComment components to better suit Security and Observability use cases. Thanks, @snide, and @cchaos for showing me pain points in the current usages.

Using the EuiTimeline

The EuiCommentList and EuiComment are now using the new EuiTimeline and EuiTimelineItem components respectively.

New props

I added a "custom" option to EuiComment.type. With this option, consumers can pass any custom element in the timeline.

In the past, the timeline was reflecting the type of event. The type update had a smaller avatar and it was defaulting to a dot. Now the timeline always defaults to the avatar with the username initial letter:

For this reason, I needed to introduce a new prop updateIcon to allow the events type update to have a custom icon. Also added a prop called updateColor that changes de panel color of an event type update.

The EuiComment.actions prop was updated to also accept a ReactNode[]. This way we ensure better spacing when multiple actions are added.

Breaking changes

I changed the 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.

Another breaking change is that EuiCommentList (<ol/>) is now required to wrap the EuiComment when EuiComment.component defaults to li.

Note: Not having any of these new requirements doesn't break the code but throws TS errors and without the wrapper, it breaks the a11y.

Docs

The docs were updated to reflect the changes and two new sections were added:

Comment type update section
A comment system section

Checklist

Check against all themes for compatibility 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~~
A changelog entry exists and is marked appropriately

Things to look out for when moving styles

Anything in the backlog that could be a quick fix for that component?
~~[ ] Convert global Sass vars to JS version like sizing~~
~~[ ] Convert component-specific Sass vars to exported JS versions~~
Convert side specific padding, margin, and position to -inline and -block (Logical properties)
Reduce specificity where possible (usually by reducing class name chaining)
Use gap property to add margin between items if using flex
~~[ ] Can any still existing .js files be converted to TS?~~
~~[ ] All animations or transitions are wrapped in euiCanAnimate~~

QA

Does the output CSS match the previous CSS / as expected
Does the rendered class read as expected

kibanamachine · 2022-03-04T18:57:50Z

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

kibanamachine · 2022-03-07T13:38:35Z

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

kibanamachine · 2022-03-07T15:49:07Z

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

snide · 2022-03-08T16:23:58Z

Hey @miukimiu. Thanks for working on this. I've actually been working on a slight rework of cases in the Security app and one thing @dimadavid and I had trouble with is why the avatar icon pattern flip flops.

In Github flows this can make sense because of the indents and "child" representation of things, but the way we use it (with a linear line) makes it a little difficult to grok. Part of me wonders if we should just keep avatars on one side, and actions on the other.

There are generally going to be three styles of feed items coming in the stream:

Actions that resolve to a person
Comments that resolve to a person
Actions that revolve to a system (like an outside system or automated event)

Another state that we needed to cover was:

Actions that are very important (like an alert) and need to be called out from the feed.

^^ we don't have a good pattern for this, and I was likely going to do it through custom css.

In any case, this screen shows a bit of the trouble:

cchaos · 2022-03-08T16:31:22Z

Let's also not forget to do any updates to the Figma Library if we change component configurations.

elizabetdev · 2022-03-09T18:51:17Z

Hi @snide and @cchaos,

Here are a few ideas to solve the issues @snide mentioned:

The avatars would always be on the same side and the update icon goes on top of the avatar.
Comments type="update" can receive an avatar and an icon that represents the update. If no update icon is passed they just get the icon="dot".
Comments type="update" can have a panel with a color (to be called out from the feed).

@snide it would be really helpful if you could give me an example of a real timeline. Just the text examples.

The Figma prototype can be found here.

Let me know what you think.

cchaos · 2022-03-09T23:39:37Z

Thanks @miukimiu ! I added comments to the FIgma.

elizabetdev · 2022-03-10T19:10:53Z

@snide @cchaos and @dimadavid just to keep the API conversation here and after going through the Figma comments here are my proposed changes:

1 - In the current implementation, the avatar with the user is the default. I propose to change it to avatar initials. The username is required so we can always get the initials. All the implementations I've seen so far use the initials. And it makes sense because is a good way to distinguish users. Why would the avatar be the default?

2 - When the EuiComment.type is update and no icon is specified it gets the dot. Why do I’m insisting on always having this update icon? Because without it then it gets a mess. Some updates get an icon... Others don’t... And visually they don’t get aligned. The icons could also be in the middle of the text. But it would add too much visual noise (IMO). They wouldn't get aligned. So I vote to let it just before the username.

3 - Be able to pass a EuiPanel.color to be called out from the feed. In these cases, I would remove the default update icon dot or any other update icon that is passed should be omitted.

4 - Custom actions should be aligned on the right side.

New icons (probably not needed)

1 - I noticed that in Observability -> Cases an arrow right is being used to show the alert details (opens in a flyout).

What should be the right icon for this scenario? I created a new one called for now documentOpen. Well... Any suggestions for what should be the right icon. Would this one work?

2 - If we change to avatars for the initials we won't require to show the icon user anymore... But I notice that we don't have an icon that represents a bot or a service. So I created a new icon called bot.

These new designs are in the prototype (pages 3 and 4). You can add your design comments there.

kibanamachine · 2022-03-15T17:35:48Z

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

kibanamachine · 2022-03-15T20:37:29Z

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

…ith-markdown-editor

kibanamachine · 2022-05-03T18:44:21Z

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

kibanamachine · 2022-05-04T12:28:39Z

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

…e additional qualifiers

chandlerprall · 2022-05-04T14:30:43Z

I re-enabled the shouldRenderCustomStyles check with an updated regex test to allow arbitrary additions to the generated classname. @constancecchen if you wouldn't mind checking that regex change in d25ed5c

kibanamachine · 2022-05-04T14:57:40Z

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

…ith-markdown-editor

kibanamachine · 2022-05-06T15:59:51Z

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

cchaos

I love the final output of this. Very uniform and easy to scan. Here is some early feedback just looking at the component code and output for now. I'm also happy to pair on any of these that aren't particularly clear.

Mobile

I would see if you can get the event details items to all wrap in some fashion otherwise they'll need sort of overflow scroll on them or something.

cchaos · 2022-05-09T19:45:34Z

src/components/timeline/timeline_item.tsx

@@ -45,7 +45,6 @@ export const EuiTimelineItem: FunctionComponent<EuiTimelineItemProps> = ({
  verticalAlign = 'center',
  icon,
  iconAriaLabel,
-  className,


👍 Nice catch

cchaos · 2022-05-09T19:54:12Z

src/components/timeline/timeline_item.styles.ts

@@ -31,6 +31,7 @@ export const euiTimelineItemStyles = ({ euiTheme }: UseEuiTheme) => ({
      }

      &:not(:only-child) > [class*='euiTimelineItemIcon-center']::before {
+        display: flex;


I think you'll find that you'll be fighting with this override quite a bit if you ever change the default display of EuiTimelineItemIcon. You may want to mvoe this set of styles to outside of :last-of-type.

src/components/comment_list/comment_timeline.tsx

cchaos · 2022-05-09T20:04:22Z

src/components/comment_list/comment_timeline.tsx

  let iconRender;
  if (typeof timelineIcon === 'string') {
    iconRender = (
-      <EuiIcon size={type === 'update' ? 'm' : 'l'} type={timelineIcon} />
+      <EuiAvatar
+        className="euiCommentTimeline"


Just looking at the rendered DOM output, this classname no longer seems appropriate. Since there's already a breaking change on this component, I'd suggest changing this to be EuiCommentIcon or EuiCommentTimelineIcon or even avatar instead of icon. But leaving it at simply timeline I think mis-represents what and where it renders.

cchaos · 2022-05-09T20:07:53Z

src/components/comment_list/comment_timeline.test.tsx

+            username="someuser"
            timelineIcon={<EuiAvatar size="l" name="Mario" />}


Hmmm, do we think this is duplicative? Mostly meaning, if username is required, but they want to pass a custom Avatar, they're most likely duplicating username on both but the timeline icon doesn't actually render / use the username anymore in the custom case.

cchaos · 2022-05-09T20:10:06Z

src/components/comment_list/comment_list.tsx

-     */
-    comments?: EuiCommentProps[];
-  };
+export type EuiCommentListProps = EuiTimelineProps & {


It's showing both items and comments props allowed, but I'd suggest restricting to just comments.

Suggested change

export type EuiCommentListProps = EuiTimelineProps & {

export type EuiCommentListProps = Omit<EuiTimelineProps, 'items'> & {

src/components/comment_list/comment_event.tsx

src/components/comment_list/comment_event.styles.ts

kibanamachine · 2022-05-12T14:59:49Z

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

cchaos · 2022-05-16T16:58:27Z

I'm pushing this PR off to Stack 8.4 as it's a major breaking change for all consumers of this component.

…ith-markdown-editor

kibanamachine · 2022-05-18T16:10:58Z

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

…ith-markdown-editor

kibanamachine · 2022-06-01T10:14:44Z

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

elizabetdev · 2022-07-06T15:11:19Z

Closing in favor of #6030

elizabetdev added 5 commits March 4, 2022 17:43

[EuiComment] Add type "custom"

239bfe2

Adding error state

662e8c6

Adding CL

ba11471

CL number

fa92dae

Adding missing array key

8da2022

All children using EuiMarkdownFormat to render

b7a37c8

elizabetdev changed the title ~~[EuiComment] Add type "custom" and comment system example~~ [EuiComment] Add "custom" option to EuiComment.type and comment system docs example Mar 7, 2022

elizabetdev added 2 commits March 7, 2022 14:36

Fixing codesandbox issue with height

36189ad

Changed .euiMarkdownEditor display to flex

770d299

elizabetdev marked this pull request as ready for review March 7, 2022 15:08

Adding unit tests for type

b794788

elizabetdev requested a review from chandlerprall March 7, 2022 15:20

cchaos self-requested a review March 8, 2022 16:16

Removing error text

bad52f5

Adding new props

b877e80

Adding docs

136d073

elizabetdev added 4 commits March 16, 2022 13:41

Text examples

6491fd2

Merge remote-tracking branch 'upstream/main' into docs-comment-list-w…

d3d756b

…ith-markdown-editor

Actions a ReactNode[]

8073f6f

Better variable naming

3bdfe60

Better emotion styles grouping

d00143c

elizabetdev added 2 commits May 4, 2022 12:49

Snippets

bb28072

Snippets

d2e4779

Extend shouldRenderCustomStyles class name regex to allow zero or mor…

d25ed5c

…e additional qualifiers

Merge remote-tracking branch 'upstream/main' into docs-comment-list-w…

3b4d601

…ith-markdown-editor

elizabetdev mentioned this pull request May 9, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

cchaos suggested changes May 9, 2022

View reviewed changes

elizabetdev added 2 commits May 11, 2022 13:55

No more comment types

a7a0ee2

Removed EuiComment.type and more changes

ba8de79

elizabetdev added 2 commits May 12, 2022 17:01

Adding logicalCSS

33993d5

Prettier

629db79

cchaos added this to the Elastic Stack 8.4 milestone May 16, 2022

elizabetdev added 2 commits May 18, 2022 16:30

Adding flexibe component section

3b93f5b

Merge remote-tracking branch 'upstream/main' into docs-comment-list-w…

252ce33

…ith-markdown-editor

elizabetdev mentioned this pull request May 20, 2022

[Emotion] Convert EuiFacetButton and EuiFacetGroup #5878

Merged

11 tasks

elizabetdev added 2 commits May 24, 2022 15:44

simplifying avatar line vertical alignment logic

c953a18

Merge remote-tracking branch 'upstream/main' into docs-comment-list-w…

9737423

…ith-markdown-editor

elizabetdev mentioned this pull request Jun 7, 2022

Add gutterSize prop to EuiTimeline #5955

Merged

8 tasks

elizabetdev closed this Jul 6, 2022

elizabetdev mentioned this pull request Jul 11, 2022

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

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiComment] Add `"custom"` option to `EuiComment.type` and more enhancements #5692

[EuiComment] Add `"custom"` option to `EuiComment.type` and more enhancements #5692

elizabetdev commented Mar 4, 2022 •

edited

Loading

kibanamachine commented Mar 4, 2022

kibanamachine commented Mar 7, 2022

kibanamachine commented Mar 7, 2022

snide commented Mar 8, 2022 •

edited

Loading

cchaos commented Mar 8, 2022

elizabetdev commented Mar 9, 2022

cchaos commented Mar 9, 2022

elizabetdev commented Mar 10, 2022

kibanamachine commented Mar 15, 2022

kibanamachine commented Mar 15, 2022

kibanamachine commented May 3, 2022

kibanamachine commented May 4, 2022

chandlerprall commented May 4, 2022

kibanamachine commented May 4, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

cchaos May 9, 2022

cchaos May 9, 2022

cchaos May 9, 2022

cchaos May 9, 2022

cchaos May 9, 2022

kibanamachine commented May 12, 2022

cchaos commented May 16, 2022

kibanamachine commented May 18, 2022

kibanamachine commented Jun 1, 2022

elizabetdev commented Jul 6, 2022

		username="someuser"
		timelineIcon={<EuiAvatar size="l" name="Mario" />}

	export type EuiCommentListProps = EuiTimelineProps & {
	export type EuiCommentListProps = Omit<EuiTimelineProps, 'items'> & {

[EuiComment] Add "custom" option to EuiComment.type and more enhancements #5692

[EuiComment] Add "custom" option to EuiComment.type and more enhancements #5692

Conversation

elizabetdev commented Mar 4, 2022 • edited Loading

Summary

Using the EuiTimeline

New props

Breaking changes

Docs

Checklist

Things to look out for when moving styles

QA

kibanamachine commented Mar 4, 2022

kibanamachine commented Mar 7, 2022

kibanamachine commented Mar 7, 2022

snide commented Mar 8, 2022 • edited Loading

cchaos commented Mar 8, 2022

elizabetdev commented Mar 9, 2022

cchaos commented Mar 9, 2022

elizabetdev commented Mar 10, 2022

New icons (probably not needed)

kibanamachine commented Mar 15, 2022

kibanamachine commented Mar 15, 2022

kibanamachine commented May 3, 2022

kibanamachine commented May 4, 2022

chandlerprall commented May 4, 2022

kibanamachine commented May 4, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

Choose a reason for hiding this comment

Mobile

cchaos May 9, 2022

Choose a reason for hiding this comment

cchaos May 9, 2022

Choose a reason for hiding this comment

cchaos May 9, 2022

Choose a reason for hiding this comment

cchaos May 9, 2022

Choose a reason for hiding this comment

cchaos May 9, 2022

Choose a reason for hiding this comment

kibanamachine commented May 12, 2022

cchaos commented May 16, 2022

kibanamachine commented May 18, 2022

kibanamachine commented Jun 1, 2022

elizabetdev commented Jul 6, 2022

[EuiComment] Add `"custom"` option to `EuiComment.type` and more enhancements #5692

[EuiComment] Add `"custom"` option to `EuiComment.type` and more enhancements #5692

elizabetdev commented Mar 4, 2022 •

edited

Loading

snide commented Mar 8, 2022 •

edited

Loading