-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(structuredProperties): refresh structured properties on update #14910
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(structuredProperties): refresh structured properties on update #14910
Conversation
Bundle ReportChanges will increase total bundle size by 1.07kB (0.0%) ⬆️. 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❌ Patch coverage is
📢 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.
overall this is making sense! i have a few questions on some of the details though
datahub-web-react/src/app/sharedV2/reloadableContext/ReloadableContext.tsx
Outdated
Show resolved
Hide resolved
datahub-web-react/src/app/entityV2/shared/entity/EntityActions.tsx
Outdated
Show resolved
Hide resolved
datahub-web-react/src/app/entityV2/shared/sidebarSection/SidebarStructuredProperties.tsx
Show resolved
Hide resolved
2621e5a to
e98d9d8
Compare
|
✅ Meticulous spotted 0 visual differences across 954 screens tested: view results. Meticulous evaluated ~9 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 0d37973. This comment will update as new commits are pushed. |
e98d9d8 to
0d37973
Compare
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.
i think we could probably have some stronger types to ensure folks use getReloadableKeyType when generating keys but this is nicer and don't need to block this moving forward
| // ChildHierarchy - to reload shown child domains on asset summary tab | ||
| reloadModules([DataHubPageModuleType.ChildHierarchy], 3000); | ||
| reloadByKeyType( | ||
| [getReloadableKeyType(ReloadableKeyTypeNamespace.MODULE, DataHubPageModuleType.ChildHierarchy)], |
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.
I think we can change ReloadableKeyTypeNamespace to ReloadableKeyType
This PR: