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

web: add expand-all and collapse-all buttons #5611

Merged
merged 2 commits into from Mar 25, 2022
Merged

web: add expand-all and collapse-all buttons #5611

merged 2 commits into from Mar 25, 2022

Conversation

nicks
Copy link
Member

@nicks nicks commented Mar 21, 2022

Hello @landism, @lizzthabet,

Please review the following commits I made in branch nicks/ch13280:

7d6cdc6 (2022-03-21 18:14:18 -0400)
web: add expand-all and collapse-all buttons

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@shortcut-integration
Copy link

@nicks
Copy link
Member Author

nicks commented Mar 21, 2022

Screenshot from 2022-03-21 18-15-05

@nicks nicks force-pushed the nicks/ch13280 branch 4 times, most recently from 100f823 to f6dd374 Compare March 22, 2022 14:35
Copy link
Contributor

@lizzthabet lizzthabet left a comment

Choose a reason for hiding this comment

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

this looks great! are you planning to add the expand and collapse all buttons to detail view in a separate pr?

edit: i realize we have a duplicate ticket in ch12977, which is what i was looking at when i was thinking about detail view buttons. 🤦🏻‍♂️ cc: @milas, i'm going the ticket we're both on.

function collapseAll(groupNames: string[]) {
let newState: GroupsState = {}
groupNames.forEach((group) => (newState[group] = { expanded: false }))
setGroups(newState)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a minor detail / edge case that i'm not sure is worth addressing, but i'll mention it anyways! when disabled resources are hidden, label groups with only disabled resources will be reset to expanded when collapseAll is invoked.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh boy it gets more complicated than that!

  • if you click "collapse all", and then create new groups, do they default to expanded or collapsed?
  • if you remove a collapsed group, click "expand all", and then bring the group back, does it default to expanded or collapsed?

i don't think there's an obvious right answer here...i like how the existing model encourages a default state of "expanded" when there are any open questions.

will leave some comments on this.

@nicks
Copy link
Member Author

nicks commented Mar 25, 2022

oh, i didn't realize that we were planning to add this to the sidebar too! ya, let's do that in a follow-up.

@nicks nicks merged commit be0a079 into master Mar 25, 2022
@nicks nicks deleted the nicks/ch13280 branch March 25, 2022 22:10
@nicks
Copy link
Member Author

nicks commented Mar 25, 2022

https://app.shortcut.com/windmill/story/13409/add-expand-collapse-buttons-to-sidebar?vc_group_by=day&ct_workflow=all&cf_workflow=500000005

also - i hope i didn't step on your toes! didn't realize there was a duplicate ticket 😬

@milas
Copy link
Contributor

milas commented Mar 28, 2022

Old duplicate ticket (ch12977) is largely my fault!

I assigned it to myself with the intent of doing it a bit back but ended up never getting to it, so no worries, and glad this made it in regardless!

@shortcut-integration
Copy link

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.

None yet

3 participants