-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Show aggregate users under groups #17495
Show aggregate users under groups #17495
Conversation
Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
Changed Packages
|
@wss-rbunning, is this opt in? As in we won’t see the switch at all if we don’t enable it. |
@ahhhndre As it is right now, the switch is not opt-in and will always be visible. It does, however, default to the off position when the page loads, showing only direct members like before. |
The general practice is to make these types of things Opt-In. Doing it this way people can choose to have it visible versus it being force upon them. Should be a pretty easy thing to implement. Can we please change that? (Sorry if this is confusing, I am Ahhhndre I just wanted to follow up on this when I saw it on my phone before I forgot and it got merged in) |
@awanlin Absolutely! I will work on making this opt-in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution @wss-rbunning, please don't take my comments about making this opt-in to seem like it's not wanted. I know that this is something the Community will be happy to have once it's merged in and released 🚀
Overall this looks good to me, left a few comments
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.tsx
Outdated
Show resolved
Hide resolved
Added usage code snippet in change set. Made the aggregate members toggle switch opt-in. Updated storybook story to have an entry for aggregate members. Refactored tests to not mock getEntityRelations. Removed usages of the word user in favor of member. Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
@awanlin I fully understand the reason for wanting to have it be opt-in and I appreciate the feedback. I have pushed changes that should address your feedback items. Please let me know if there is any other feedback or suggestions you would like me to address 👍. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
@awanlin: With how it is now, it loads the aggregated users in the background, not delaying the page load while this happens. If the switch is flipped to "Aggregate Users" before all aggregate users are done loading, no additional users will be displayed until it is finished. I am going to change this so that it will only run the logic to get aggregated users if the "show the switch" prop is set (so it isn't doing a lot of extra stuff that someone isn't asking for). Do we want to show a loading bar if the switch is flipped and aggregate users are loading? |
Sounds good to me @wss-rbunning, I think showing a progress bar would be good. How do they deal with that for the Ownership Card? They do something very similar. We don't use it though so I don't remember if it has a progress bar or not. |
Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
Uffizzi Preview |
Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
@awanlin It looks like in the Ownership Card they have a separate "ownership grid" component that is a child of the card itself and does not contain the toggle switch. This grid shows a loading bar when it is loading members. I have pushed changes with my suggestions. Here is the new behavior:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks good! 👍
Just a couple of nits
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.test.tsx
Outdated
Show resolved
Hide resolved
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.stories.tsx
Outdated
Show resolved
Hide resolved
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.tsx
Outdated
Show resolved
Hide resolved
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.tsx
Outdated
Show resolved
Hide resolved
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.tsx
Outdated
Show resolved
Hide resolved
…Card.test.tsx Co-authored-by: Patrik Oldsberg <poldsberg@gmail.com> Signed-off-by: Robert Bunning <129326788+wss-rbunning@users.noreply.github.com>
…Card.stories.tsx Co-authored-by: Patrik Oldsberg <poldsberg@gmail.com> Signed-off-by: Robert Bunning <129326788+wss-rbunning@users.noreply.github.com>
…Card.tsx Co-authored-by: Patrik Oldsberg <poldsberg@gmail.com> Signed-off-by: Robert Bunning <129326788+wss-rbunning@users.noreply.github.com>
…as a dependency in MembersListCard Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
Signed-off-by: Robert Bunning <rbunning@webstaurantstore.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you! 😁
Just a tiny think that I'll tweak and then good to
plugins/org/src/components/Cards/Group/MembersList/MembersListCard.stories.tsx
Outdated
Show resolved
Hide resolved
…Card.stories.tsx Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
This PR adds a toggle switch to the members card, allowing you to toggle between showing direct members and aggregated members of a group. Aggregated users is recursive and handles duplicated users and child groups.
This addresses feature request #16959
Before:
After:
✔️ Checklist
Signed-off-by
line in the message. (more info)