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

Timeline: allow to group multiple entries of same kind into a single node that can expand #145294

Open
bpasero opened this issue Mar 17, 2022 · 5 comments
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities timeline Timeline view issues timeline-local-history
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Mar 17, 2022

This is to explore whether the timeline view could aggregate multiple entries of the same source under a single node to reduce the spam from local history.

Examples from other products:

image

image

Notes

  • entries should still be sorted from newest to oldest even when under a parent node
  • relative time should be visible as with top level entries
  • parent node should show most recent entries time

//cc @JacksonKearl @daviddossett @misolori

@bpasero bpasero added feature-request Request for new features or functionality timeline Timeline view issues timeline-local-history labels Mar 17, 2022
@bpasero bpasero added this to the March 2022 milestone Mar 17, 2022
@bpasero bpasero self-assigned this Mar 17, 2022
@bpasero
Copy link
Member Author

bpasero commented Mar 20, 2022

This turned out to be a surprisingly small change so I pushed for it:

Recording 2022-03-20 at 10 09 45

@daviddossett @misolori fyi, happy for feedback:

  • I ended up using the same icon because in most cases the user will see the aggregated version and not a single entry and I quite like that icon already
  • number of aggregated entries is shown as part of the label
  • there is no tooltip and no command other than expand/collapse on such an entry, but we could enrich this

//cc @JacksonKearl this works by making Timeline allow to return children:

/**
* Optional support for grouping multiple timeline items
* under a parent timeline, e.g. to flatten entries of
* the same kind.
*/
children?: TimelineItem[];

bpasero added a commit that referenced this issue Mar 20, 2022
@bpasero
Copy link
Member Author

bpasero commented Mar 20, 2022

Oh, I had to revert that change, I forgot that the aggregation should not happen at the level of the provider but needs to happen inside the timeline view because we want to aggregate over all items from all sources and not within source. Otherwise the timeline would no longer show a real timeline with entries one after the other...

@bpasero bpasero modified the milestones: March 2022, April 2022 Mar 21, 2022
@daviddossett
Copy link
Contributor

Looking good! Is it possible to use an explicit tree parent with open/close? If I didn't know what the behavior was, I wouldn't know it was clickable to show the actual entries. E.g.

CleanShot 2022-03-22 at 11 27 51@2x

number of aggregated entries is shown as part of the label

I wonder if a badge a la SCM could also work for here. The plain text in brackets is clear, but feels somewhat bespoke here since we have similar patterns for counting children elsewhere:

CleanShot 2022-03-22 at 11 25 17@2x

It could look like:

CleanShot 2022-03-22 at 11 28 58@2x

@bpasero
Copy link
Member Author

bpasero commented Mar 22, 2022

👍 for both suggestions

@bpasero bpasero modified the milestones: April 2022, Backlog Apr 1, 2022
@bpasero bpasero removed their assignment Apr 1, 2022
@bpasero
Copy link
Member Author

bpasero commented Apr 1, 2022

While this is very cool, I will let this sink for a little longer and wait for user feedback. Interestingly so far I did not hear the need from users, but it is also quite early.

If someone wants to jump ahead and take an attempt at this, here are some pointers:

  • the original naive attempt I did where timelinePane sees the necessary changes to show a tree node
  • however my attempt had the flaw that this grouping was controlled from the provider, but it needs to be implemented from the timeline itself
  • I think *getItems() needs to be changed to enable grouping in a way that for as long as entries of the same kind would be yielded, they should group under 1 node
  • this needs some kind of new API on the level of timeline item to signal opt-in to this

The challenge is probably within *getItems() to implement this while still supporting yield, so it would require some kind of peeking as well, -> fun.

@bpasero bpasero added the help wanted Issues identified as good community contribution opportunities label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities timeline Timeline view issues timeline-local-history
Projects
None yet
Development

No branches or pull requests

2 participants