-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
chore: Dataset specific MetadataBar #23429
Conversation
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.
Thank you for the PR @kgabryje. I left some comments.
}; | ||
|
||
export const DatasetSpecific = () => { | ||
SupersetClient.reset(); |
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.
Can we mock useApiV1Resource
instead of introducing a new dependency (storybook-addon-mock
) and configuring SupersetClient
?
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.
Do you have a reference on how to mock a return value of a function in storybook?
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.
There's an addon to mock imports but that would also introduce a new dependency. It seems it's not very straightforward though. While thinking about the problem, I'm not sure if a storybook for this hook is highly valuable given that the MetadataBar
component already has one that displays the dataset information. I see a test file much for valuable as the objective of the hook is to assemble the correct information from the server and transform data to be displayed. If you asked me, I would delete the storybook and the new dependency and add a test file with the different combinations of server responses (no description, no creator, no owners, etc). I'll leave the decision to you as it's not a blocker for me.
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.
Added UT. Like you said, to mock a dependency we'd still need an addon.
I'd rather keep the addon for mocking API responses as I think it opens a possibility to write stories for more complex components and features.
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/MetadataBar/dataset/useDatasetMetadataBar.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/MetadataBar/dataset/useDatasetMetadataBar.tsx
Outdated
Show resolved
Hide resolved
283f1d0
to
07c610d
Compare
07c610d
to
e434100
Compare
e434100
to
d65be2b
Compare
Codecov Report
@@ Coverage Diff @@
## master #23429 +/- ##
=======================================
Coverage 67.64% 67.65%
=======================================
Files 1908 1909 +1
Lines 73739 73743 +4
Branches 7988 7988
=======================================
+ Hits 49879 49888 +9
Misses 21814 21814
+ Partials 2046 2041 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM. Thank you for the added test!
superset-frontend/src/features/datasets/metadataBar/useDatasetMetadataBar.test.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/features/datasets/metadataBar/useDatasetMetadataBar.test.tsx
Outdated
Show resolved
Hide resolved
…MetadataBar.test.tsx Co-authored-by: Michael S. Molina <[email protected]>
…MetadataBar.test.tsx Co-authored-by: Michael S. Molina <[email protected]>
SUMMARY
Metadata bar component for dataset will be used in Drill to detail modal and in Drill by modal. In order to DRY the code, this PR moves the boilerplate for constructing a dataset specific metadata bar to a separate hook.
In addition, this PR adds a storybook for the new not-generic metadata bar.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Verify that the metadata bar in drill to detail modal works like before
ADDITIONAL INFORMATION