eui icon indicating copy to clipboard operation
eui copied to clipboard

[EuiCommentList] Add ability to collapse multiple events

Open i-a-n opened this issue 3 years ago • 19 comments

Okay hear me out. This might sound a bit out there, but in our use-case of EuiCommentList, we'd really like the ability to hide the gray vertical bar that stitches together timeline icons. Like so:

Screen Shot 2021-09-03 at 2 26 05 PM

Our previous, hand-rolled comment list component allowed us to do this, and it was very useful for one particular case: When a user is using a provided search bar to filter through a comment list, we automatically surface only the matching comments: it no longer makes sense to show one following the other in a timeline-like format, and removing the gray bar helped us achieve that.

Is that something you'd be interested in pursuing?

i-a-n avatar Sep 03 '21 21:09 i-a-n

it no longer makes sense to show one following the other in a timeline-like format

I actually disagree with this sentiment, unless you are somehow providing a "closest match" first approach to display order, which would cause problems itself. But if you're providing search capability to narrow down comments, what you're really doing is filtering "out" comments that don't match the given criteria. There is still an inherit order to the display of the comments that should be maintained and therefore the line continues this idea. What you really want to be doing is saying "There are comments between these events that are hidden". By doing something like this:

image

cchaos avatar Sep 04 '21 20:09 cchaos

excellent point @cchaos , that's right on the money 🎯

I suppose then conceptually what I'd ask for in the EuiCommentList API is a way to pass that visual indicator between known comments, like:

[
  { comment },
  { comment },
  { visualIndicator },
  { comment },
  ...
]

although that feels like a pretty heavy lift for such a non-core functionality. alternatively, if the EuiCommentList DOM structure allows it, do you think it might be possible to do something on the client-side like:

[
  { comment },
  { comment, className: injectVisualIndicatorAfterThisComment },
  { comment },
]

...and then absolute position our own visual indicator? if so I'd be happy to close this issue and work on that for our use-case.

i-a-n avatar Sep 07 '21 16:09 i-a-n

I think it's fair to request something like a collapsed indicator from the EUI component itself. This could be useful in more than just your specific filter/search use-case, but for things like wanting to hide bot-style events. We'd just have to decide how best to collapse multiple events into a single indicator.

cchaos avatar Sep 07 '21 16:09 cchaos

Here's a better mock for how we can create a collapsed version of EuiComment.

image

@i-a-n How are you then supplying the object to the EuiCommentList? Are/Do you want to completely remove the items and instead replace blocks with a single EuiComment type="collapsed". Or would you like to add a new key/prop on the ones to hide like collapsed={true}?

cchaos avatar Sep 08 '21 19:09 cchaos

@cchaos at current we supply EuiCommentList with only the matching comments, which we are asking EuiSearchBar to return:

        searchResults: EuiSearchBar.Query.execute(
          query,
          this.props.caseComments,
          {
            defaultFields: ['text', 'author.name'],
          }
        )

...so we'd likely have to re-engineer how we utilize EuiSearchBar (or, evaluate whether we can even still use it) if we want the non-matching comments to be flagged somehow, instead of simply being filtered out.

i-a-n avatar Sep 08 '21 21:09 i-a-n

oh and, one other data point before I forget: we rolled our own "show older comments" button, but if collapsing/expanding comments can be configured to handle incremental revealing, I'd personally love to use that instead.

Screen Shot 2021-09-08 at 2 19 54 PM

i-a-n avatar Sep 08 '21 21:09 i-a-n

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

github-actions[bot] avatar Mar 08 '22 00:03 github-actions[bot]

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

github-actions[bot] avatar Sep 04 '22 16:09 github-actions[bot]

Hey team, response-ops-cases team is using EuiCommentList in the Cases page to show user actions in Kibana. These user actions can be up to 10K. Do you think it will be good idea in this case to show top 10 comments and bottom 10 comments and collapse other comments in the middle? Also are you planning to show all hidden comments when user clicks on it? If not, is it possible to provide option to show 10 more and still keep other comments hidden?

js-jankisalvi avatar Mar 09 '23 15:03 js-jankisalvi

You may already be working with @mdefazio on this, but tapping him in for awareness. I'm pretty certain he has spent some time thinking on this UI, as well.

ryankeairns avatar Mar 09 '23 15:03 ryankeairns

@js-jankisalvi the design we have doesn't consider cases for up to 10k comments. So we have to rethink the design to also handle those scenarios.

We'll consider your suggestions. I'll also make sure to talk with @mdefazio.

miukimiu avatar Mar 09 '23 15:03 miukimiu

Here is @js-jankisalvi PR with a demo of what she described: https://github.com/elastic/kibana/pull/152702.

cnasikas avatar Mar 09 '23 17:03 cnasikas

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

github-actions[bot] avatar Oct 22 '23 00:10 github-actions[bot]

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.

github-actions[bot] avatar Apr 19 '24 08:04 github-actions[bot]

Hey y'all - it looks like this issue/thread got completely dropped after the departure of the EUI designers on it. @cnasikas I see you emoji reacting to the stale bot - is this feature request still a priority for y'all that you'd like us to triage/see across the finish line?

CC @JasonStoltz

cee-chen avatar Apr 22 '24 17:04 cee-chen

Hey @cee-chen.

We implemented the feature in Cases ourselves but it would be nice to be supported natively by the EUI library. It is not a high priority if there is no capacity in your team at the moment. Should we keep it open for visibility?

cnasikas avatar Apr 23 '24 11:04 cnasikas

@cnasikas Do you mind pointing us to your Cases implementation?

JasonStoltz avatar Apr 25 '24 13:04 JasonStoltz

@JasonStoltz here's a design file that illustrates it (let me know if you're having issues viewing)

https://www.figma.com/file/W6BWTS3dVv7SLTcAXzprP4/Case-Attachments?type=design&node-id=2159-315345&mode=design&t=kI0U8E1OtjSRSe5G-11

mdefazio avatar Apr 25 '24 15:04 mdefazio

Thank you @mdefazio! @JasonStoltz In Kibana, if you go to Cases and produce more than 20 user actions (every update you do on a case will produce a user action) you will see a "Show more" button. What we do is that we fetch the first 10 user actions and the last 10 user actions. When the user presses the "Show more" button we fetch the next 10 from the start. This continues until there are no user actions left. We do not fetch all user actions and just hide them.

cnasikas avatar Apr 26 '24 07:04 cnasikas

We discussed as a team whether we'd want to adopt this into the core EUI library. We think this is fine as a custom solution and are hesitant to add it to the core library. However, it would be useful to call this out as a pattern to keep other future implementations visually aligned. CC @kyrspl.

JasonStoltz avatar May 01 '24 15:05 JasonStoltz

Thank you @JasonStoltz! Make sense.

cnasikas avatar May 02 '24 08:05 cnasikas