Refactor types in CollectionPicker #40377
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kamilmielnik
commented
Mar 21, 2024
| @@ -26,6 +24,12 @@ const canSelectItem = (item: CollectionPickerItem | null): boolean => { | |||
| return !!item && item?.can_write !== false; | |||
| }; | |||
|
|
|||
| const searchFilter = ( | |||
There was a problem hiding this comment.
This change is not type-related.
This function does not need to be a hook.
This reverts commit b762d0f.
|
ranquild
reviewed
Mar 21, 2024
ranquild
approved these changes
Mar 21, 2024
iethree
approved these changes
Mar 21, 2024
There was a problem hiding this comment.
Very nice work 👏 Thanks! As you can see, we struggled a bit with the types 🥴
I think that the complexity of these types is actually a smell that the diversity of API responses we get for the same things from the backend could use some work. I'm hopeful that the openAPI docs the backend has recently put in, along with some of the code gen features of RTK Query can help us simplify this in the future.
1 task
|
@kamilmielnik Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Needed for #40298
Description
EntityPickercontains a search component which has been build to support all types of models (seeSearchModelType).However - depending on the context - it will usually be used only with a subset of models, for example:
Up until now
EntityPickertype declarations were tightly coupled with Collections.This PR removes this coupling and makes the component generic so that we can have type safety in various context without type casting. It's mostly about moving files and extending the types, but I'm sneaking in a few small code changes as well.
Short summary of changes:
metabase/common/components/EntityPickertometabase/common/components/CollectionPickermetabase/common/components/EntityPicker/generic, so that it can be used in various contextsEntityPickerSearchand 1 in a unit testanyssearchFilterfunction (it didn't have to be a hook)NestedItemPickerstorybook page - it didn't work and was using an outdated interface ofNestedItemPicker. Maybe it used to deserve a storybook page at the time of creation, but I don't think it does anymore as its UI part now lives inItemList.How to verify
metabase/common/components/CollectionPickerCollectionPickeris Collections-specific