-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
fix: Dataset creation header is now uneditable and holds proper default values #21557
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21557 +/- ##
==========================================
+ Coverage 66.65% 67.00% +0.35%
==========================================
Files 1797 1799 +2
Lines 68752 70080 +1328
Branches 7325 7807 +482
==========================================
+ Hits 45829 46960 +1131
- Misses 21049 21178 +129
- Partials 1874 1942 +68
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 |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://18.236.78.16:8080. Credentials are |
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.
Big fan of the way you improved/simplified the tests! 🎉
); | ||
|
||
const LeftPanelComponent = () => ( | ||
<LeftPanel | ||
setDataset={setDataset} | ||
schema={dataset?.schema} | ||
schema={dataset?.schema ?? ''} |
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.
Since schema
is optional as per the definition in LeftPanelProps
wouldn't be sufficient to just send dataset?.schema
? without the empty string.
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, that does make more sense! Fixed in this commit
.
waitFor(() => | ||
render(<Header setDataset={mockSetDataset} datasetName="" />), | ||
); | ||
waitFor(() => render(<Header setDataset={mockSetDataset} title="" />)); |
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.
Isn't this the same as having no title thus the title: string;
should be optional? Or what implication would that have? 🤔
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.
The title can't have nullish values because of the component that uses it: PageHeaderWithActions
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 the other comments I left might address this and allow the title="" to be removed
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx
Outdated
Show resolved
Hide resolved
7fa31ba
to
96235fe
Compare
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://35.91.137.104:8080. Credentials are |
title: datasetName, | ||
placeholder: t('Add the name of the dataset'), | ||
title: schema ? title : t('New dataset'), | ||
placeholder: t('New dataset'), |
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.
Since you are using t'(New dataset') twice here, it may be worth defining a const with that value and using it in both places. If you export that const you could also use it in the test file so that if someone changes the value later you don't have to keep three separate string literals in sync manually.
export const DEFAULT_TITLE = t(New dataset);
Then use that on line 70, 71, and in line 51 of Header.test.tsx
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.
Updated in this commit
}) { | ||
const editableTitleProps = { | ||
title: datasetName, | ||
placeholder: t('Add the name of the dataset'), | ||
title: schema ? title : t('New dataset'), |
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.
curious why the usage of title here is tied to if schema is defined? If you make title optional, then set a default value for title as t('New dataset') could the conditional logic here could be removed?
export const DEFAULT_TITLE = t(New dataset); export default function Header({ setDataset, title = DEFAULT_TITLE, }: { setDataset: React.Dispatch<DSReducerActionType>; title?: string; }) { const editableTitleProps = { title, placeholder: DEFAULT_TITLE
It looks like the only use of schema prop is this case so changing this logic might also make the prop unnecessary and simplify the component's interface
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.
Updated in this commit
<Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} /> | ||
<Header | ||
setDataset={setDataset} | ||
title={dataset?.table_name ?? 'New dataset'} |
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.
May want to wrap in a t('New dataset')
Also, may be worth using exported const from previous comment if you want to keep this in sync with what the default title is for header.tsx.
If making the title prop optional like mentioned in comment on Header/index.tsx then this could be simplified to title={dataset?.table_name}
and the default value 'New dataset' would get applied by Header and schema prop could be removed
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.
Updated in this commit
waitFor(() => | ||
render(<Header setDataset={mockSetDataset} datasetName="" />), | ||
); | ||
waitFor(() => render(<Header setDataset={mockSetDataset} title="" />)); |
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 the other comments I left might address this and allow the title="" to be removed
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.
Looks great :)
It's looking so beautiful 😍 |
/testenv up |
@jinghua-qa Ephemeral environment spinning up at http://54.202.251.181:8080. Credentials are |
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!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Dataset header name is now uneditable. It displays "New dataset' until a table is selected, then it displays the selected table's name.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
TESTING INSTRUCTIONS
http://localhost:9000/dataset/add/?testing
ADDITIONAL INFORMATION