Skip to content

Conversation

evanpurkhiser
Copy link
Member

This hook may be used to load member and team options for a select component.

This hook may be used to load member and team options for a select component.
@evanpurkhiser evanpurkhiser requested a review from a team as a code owner April 18, 2024 22:38
@evanpurkhiser evanpurkhiser requested review from davidenwang and a team and removed request for a team April 18, 2024 22:38
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 18, 2024
Copy link

codecov bot commented Apr 18, 2024

Bundle Report

Changes will increase total bundle size by 843 bytes ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 26.22MB 843 bytes ⬆️

/**
* Props to pass to the leading avatar component
*/
avatarProps?: BaseAvatarProps;
Copy link
Member

Choose a reason for hiding this comment

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

This seems misplaced. It's weird imo for a hook to return react components and it's a code smell that this function call is just passing along this object into the component. To me is means that there are two jobs happening inside this hook.

What's also a code smell is asking: could someone use this hook for anything other than the SentryMemberTeamSelectorField component and the answer is probably no. We're not returning lists of members + my teams + other teams + disabled teams.... instead it's returning a list of avatars under each category which limits re-use a lot.

I'd make changes to that one file is responsible for importing , and , and the hook is responsible for combining the data fetching, and returns one isFetching or isLoading value along with the arrays of objects and filter callbacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

SentryMemberTeamSelectorField component and the answer is probably no

I'll be using this for a Owners component as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah I can adjust it a bit to move the option generation to a diff hook

Copy link
Member Author

Choose a reason for hiding this comment

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

@ryan953 mind if I get this in as is now actually and I'll get a follow up PR up where it separates the options transformation logic?

I'd like to get the owners filter in asap today too

Copy link
Member

Choose a reason for hiding this comment

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

yup, sounds good to me!

@ryan953 ryan953 requested a review from a team April 19, 2024 16:53
@evanpurkhiser
Copy link
Member Author

Splitting out over to here actually #69348

@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/ref-ui-extract-useowneroptions branch April 19, 2024 20:21
evanpurkhiser added a commit that referenced this pull request Apr 19, 2024
This hook may be used to load member and team options for a select
component.

This is a spin off of #69269

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
MichaelSun48 pushed a commit that referenced this pull request Apr 25, 2024
This hook may be used to load member and team options for a select
component.

This is a spin off of #69269

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants