-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(context): Various UI improvements for Context Base (Part 1/2) #15413
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
Conversation
|
✅ Meticulous spotted 0 visual differences across 998 screens tested: view results. Meticulous evaluated ~8 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 48bbce0. This comment will update as new commits are pushed. |
Bundle ReportChanges will increase total bundle size by 10.13kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
chriscollins3456
left a comment
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 this makes sense. there's a few things that could use some cleanup and i would love to have consensus on what entity select we're using going forward
nothing blocking approval though!
| // Reset to initial state when closing | ||
| setSelectedUrns(initialSelectedUrns); | ||
| } catch (error) { | ||
| console.error('Failed to update related entities:', error); |
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.
could show the user an error here
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.
good idea!
| const userContext = useUserContext(); | ||
|
|
||
| // Entity types that can be related (based on RelatedAsset.pdl) | ||
| // I don't love this, but I do want to enable searching for documents |
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.
yeah i feel that we will definitely forget to add something to this list in the future..
that being said i don't see an obvious solution without muddying up our API for a specific use case like this
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.
Agree..
| relatedAssets?: DocumentRelatedAsset[]; | ||
| } | ||
|
|
||
| export const RelatedAssetsSection: React.FC<RelatedAssetsSectionProps> = ({ relatedAssets }) => { |
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.
is this component still used after replacing with RelatedSection?
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.
Good call - removing!
| @@ -34,7 +34,7 @@ export const RelatedDocumentsSection: React.FC<RelatedDocumentsSectionProps> = ( | |||
|
|
|||
| return ( | |||
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.
same - is this used elsewhere still?
| * Handles all search logic internally and provides a clean interface. | ||
| * Includes the Dropdown wrapper, so consumers don't need to wrap it themselves. | ||
| */ | ||
| export const EntitySearchDropdown: React.FC<EntitySearchDropdownProps> = ({ |
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.
at some point soon we gotta consolidate everything to the final entity selection dropdown and pick our best implementation. too many floating around out there it's hard to know what to use
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.
Yes agreed. This is the one i've used for the newer Access Requests feature (so users can search + select an entity) and now this feature -but those are the only two use cases that are curretly using this..
…atahub-project#15413) Co-authored-by: John Joyce <[email protected]>
…atahub-project#15413) Co-authored-by: John Joyce <[email protected]>
Summary
In this PR:
Screenshots