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

feat: add GroupListPicker component #14005

Merged
merged 27 commits into from
Nov 4, 2022
Merged

feat: add GroupListPicker component #14005

merged 27 commits into from
Nov 4, 2022

Conversation

djamaile
Copy link
Collaborator

@djamaile djamaile commented Oct 5, 2022

Signed-off-by: djamaile rdjamaile@gmail.com

Hey, I just made a Pull Request!

new

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

Signed-off-by: djamaile <rdjamaile@gmail.com>
@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Oct 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-org-react plugins/org-react minor v0.0.0

djamaile and others added 9 commits October 6, 2022 09:34
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: Mathias Bronner <mathiasb@spotify.com>
Signed-off-by: Mathias Bronner <mathiasb@spotify.com>
Signed-off-by: Mathias Bronner <mathiasb@spotify.com>
Signed-off-by: Mathias Bronner <mathiasb@spotify.com>
@djamaile djamaile marked this pull request as ready for review October 6, 2022 12:24
@djamaile djamaile requested review from a team as code owners October 6, 2022 12:24
Signed-off-by: djamaile <rdjamaile@gmail.com>
Copy link
Member

@tudi2d tudi2d left a comment

Choose a reason for hiding this comment

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

like this - some smaller things - hopefully I'm not to picky here. In general - did you consider splitting up the component in e.g. a GroupListPickerPopover (containing only the Popover & relevant state) & GroupListPicker? :)

…Picker.tsx

Co-authored-by: Philipp Hugenroth <tudi2d@users.noreply.github.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If you are the author and the PR has been closed, feel free to re-open the PR and continue the contribution!

@github-actions github-actions bot added the stale label Oct 14, 2022
@djamaile
Copy link
Collaborator Author

PRs can get stale?! Def not stale, just waiting on a review.

@github-actions github-actions bot removed the stale label Oct 14, 2022
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 Couple of nits

Another thing I'm wondering is how you feel about the placement of this in catalog-react vs something like org-react. It feels a bit off to have kind-specific components in catalog-react tbh, it's already a pretty large library. It also conflicts quite heavily with the UserListPicker, which is a different type of component that helps filter catalog index pages.

.changeset/hungry-rocks-bathe.md Outdated Show resolved Hide resolved
options={groups ?? []}
groupBy={option => option.spec.type}
getOptionLabel={option =>
option.spec.profile?.displayName ?? option.metadata.name
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to include the namespace here as well if it's not default. humanizeEntityRef can help out there

});

return groupsList.items as GroupEntity[];
}, [catalogApi]);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to depend on the groupTypes as well right?

*
* @public
*/
export type GroupListPickerProps = {
Copy link
Member

Choose a reason for hiding this comment

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

How is one supposed to get a hold of the selected group? 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't it the defaultGroup prop? Or do you mean something different? 💭

Copy link
Member

Choose a reason for hiding this comment

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

As someone using this component in my own page, how do I know what group the user ended up picking?

@djamaile
Copy link
Collaborator Author

@Rugvip I agree that catalog-react might be an awkward place. Do you mean creating a new plugin called org-react or just the org plugin?

@Rugvip
Copy link
Member

Rugvip commented Oct 18, 2022

@djamaile yep a new org-react package. Same relation between that and org as between catalog-react and catalog. They're not separate plugins, but library packages provided by the plugins for others to use.

Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
@@ -49,6 +49,7 @@ yarn.lock @backstage/reviewers @backst
/plugins/kubernetes @backstage/reviewers @backstage/warpspeed
/plugins/kubernetes-* @backstage/reviewers @backstage/warpspeed
/plugins/newrelic-dashboard @backstage/reviewers @mufaddal7
/plugins/org-react @backstage/reviewers
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Rugvip Is this the right owner? Does it need an owner at all?

Copy link
Member

Choose a reason for hiding this comment

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

You can skip the explicit owner, there's a catch-all at the bottom that handles this

@djamaile djamaile requested a review from Rugvip October 18, 2022 16:12
Signed-off-by: djamaile <rdjamaile@gmail.com>
@djamaile
Copy link
Collaborator Author

@Rugvip its all green, feel free to review again 😸

Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

🎉

CC @backstage/maintainers for the addition of the org-react package

plugins/org-react/src/plugin.ts Outdated Show resolved Hide resolved
@Rugvip Rugvip added the needs discussion Bring up for discussion during next sync label Oct 19, 2022
Signed-off-by: djamaile <rdjamaile@gmail.com>
Signed-off-by: djamaile <rdjamaile@gmail.com>
@Rugvip Rugvip removed the needs discussion Bring up for discussion during next sync label Oct 20, 2022
@djamaile djamaile requested a review from Rugvip October 21, 2022 07:34
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Another round! 🍻

plugins/org-react/README.md Outdated Show resolved Hide resolved
plugins/org-react/src/routes.ts Outdated Show resolved Hide resolved
Signed-off-by: djamaile <rdjamaile@gmail.com>
@djamaile djamaile requested a review from Rugvip October 26, 2022 07:58
@mathiasbronner
Copy link
Contributor

whats the status now btw @Rugvip @djamaile? Just asking because i'm looking forward to using it soon 🥳

@djamaile
Copy link
Collaborator Author

@mathiasbronner Sorry, for the long wait! Besides me making silly mistakes we also had our offsite + BackstageCon in Detroit. So, things have been slower than normal. I hope to get this in the next release 😄

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Nov 2, 2022
@Rugvip Rugvip removed the stale label Nov 2, 2022
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Let's :shipit: 😁

@Rugvip Rugvip merged commit 4f9607f into backstage:master Nov 4, 2022
@suuus
Copy link
Contributor

suuus commented Nov 24, 2022

Thank you for contributing during Hacktoberfest 🍂! You can claim the Backstage Hacktoberfest Holopin at https://bck.st/hacktoberfest-holopin 🙌🏻

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.

5 participants