-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Create dataset blank state #21058
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21058 +/- ##
==========================================
+ Coverage 66.27% 66.36% +0.09%
==========================================
Files 1769 1775 +6
Lines 67475 67726 +251
Branches 7171 7223 +52
==========================================
+ Hits 44716 44948 +232
- Misses 20928 20934 +6
- Partials 1831 1844 +13
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 |
@pkdotson Ephemeral environment spinning up at http://35.166.252.57: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.
This looks great, I just have some questions regarding overall strucutre and a small nit in regards to DatasetLayout naming.
footer?: ReactElement<any, string | JSXElementConstructor<any>> | null; | ||
} | ||
|
||
export default function DatasetLayout({ |
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 page replacing the index.tsx for DatasetPage? If so, would it make sense that this be a) named index.tsx to follow the pattern of the other pages. b) Should we move the reducer into this file?
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 DatasetLayout structures the DatasetPage, see here: https://github.com/apache/superset/pull/21058/files#diff-20b7cfe4750d43aa2c1756a68fb109b40b812464206ac3565b2c8865a5ba39e5R71-R76
This is the component we'll use to structure the edit dataset page as well, so it'll all be in the same page layout. You just need to pass in the components.
import { render, screen } from 'spec/helpers/testing-library'; | ||
import DatasetPage from 'src/views/CRUD/data/dataset/DatasetPage'; | ||
|
||
describe('DatasetPage', () => { |
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.
what is the difference between DatasetPage, DatasetPanel, and DatasetLayout in this current setup?
In the original setup, I believe that DatasetPage was the overarching structure, DatasetPanel was the middle component w/table info and DatasetLayout didn't exist. It seems like DatasetLayout is replacing DatasetPage's function. Is that true? If so, does it make sense to have these three similarly named pages/what is each one doing?
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.
- DatasetPage is the whole "add dataset" page
- DatasetLayout structures all of the components on the page.
- DatasetPanel is the center component currently holding the "Select Dataset Source" blank state
I think if we were to rename them I'd suggest something like:
- DatasetPage -> AddDatasetPage
- Since there will also be an EditDatasetPage
- DatasetLayout -> Either DatasetPageLayout or keep it as is, I think it describes it properly
- DatasetPanel -> Could probably leave this name as is as well, but I'm open to suggestions 😁
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.
Ok, that makes sense. Maybe we should make DatasetPage into just AddDataset. I also think that we should put DatasetLayout into its own folder, with an index.tsx and a testing file.
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, changed in this commit
.
<span | ||
role="button" | ||
onClick={() => { | ||
window.location.href = `/superset/sqllab`; |
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.
worth checking w/ @yousoph if we want this to open into a new tab or in the same page. For context, currently it opens in the same tab, which I prefer.
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 figma says same tab here.
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://18.237.97.55:8080. Credentials are |
Closing and reopening to reset ephemeral environment. |
Ephemeral environment shutdown and build artifacts deleted. |
/testenv up |
@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete. |
@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details. |
/testenv up |
Closing to reset ephemeral environment. |
Ephemeral environment shutdown and build artifacts deleted. |
/testenv up |
@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete. |
@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details. |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://34.218.47.237:8080. Credentials are |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://52.24.233.127:8080. Credentials are |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://54.149.181.250: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. can we merge?
<> | ||
{t('Datasets can be created from database tables or SQL queries. Select ')} | ||
<br /> | ||
{t('a database table to the left or ')} |
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.
any reason why this isn't in a p tag and wrapping on its own?
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 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.
Fixed in this commit
.
</span> | ||
{t(' to open ')} | ||
<br /> | ||
{t('SQL Lab. From there you can save the query as a 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.
Same here on these.
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.
Fixed in this commit
.
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This PR implements:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
TESTING INSTRUCTIONS
http://localhost:9000/dataset/add/?testing
ADDITIONAL INFORMATION