-
Notifications
You must be signed in to change notification settings - Fork 14k
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: Enable new dataset creation flow #22610
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22610 +/- ##
==========================================
+ Coverage 67.09% 67.34% +0.25%
==========================================
Files 1869 1875 +6
Lines 71605 72086 +481
Branches 7821 7854 +33
==========================================
+ Hits 48043 48547 +504
+ Misses 21534 21514 -20
+ Partials 2028 2025 -3
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.
Thanks, left some comments!
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/DatasourcePanel/index.tsx
Outdated
Show resolved
Hide resolved
...rset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
Outdated
Show resolved
Hide resolved
...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
Outdated
Show resolved
Hide resolved
One more comment from @yousoph: errors in the new dataset flow should show up as toasts for now; will circle back if that should change for new dataset flow. |
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.
Thanks! Left a few more comments. Redirect to Add Charts and error toasts look great.
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx
Show resolved
Hide resolved
…mponents/EmptyState
…nslations, change window.location.href redirect to use history.push()
a72d058
to
06363d9
Compare
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.
One (last, I swear) thing I just noticed is that in the previous modal, saving a dataset would redirect you to /chart/add
with a ?dataset=table_name
query string, which would autoselect the just-created dataset and also show a toast on the Add Chart page. If this behavior is easy to reproduce it should probably be maintained, but feel free to do it in a separate PR. Database autoselect now works great and everything else LGTM!
Easy fix, thanks for that catch! Fixed in |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://18.237.97.3:8080. Credentials are |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://35.89.232.0:8080. Credentials are |
Hi, found an issues, an error occurs when you go to the "create dataset" page after you have deleted the newly connected database, also, the deleted database is displayed in the dropdown list error.message.mp4 |
after we have created the dataset, it should be marked as "already exists" and the button should be disabled on "create dataset" page, but this does not happen Repro steps:
already.exists.mp4 |
Hey @andrey-zayats , thanks for the review! I've just pushed up fixes for this issue and the "already existing" issue (here). The other issues (slow-loading SQL Lab and disappearing top nav in SQL Lab) are unrelated to my changes. |
Ephemeral environment shutdown and build artifacts deleted. |
Hi @lyndsiWilliams! Can you move |
Can do! I'll be moving all dataset files to the new structure within the next couple of weeks. |
This reverts commit c87f654.
This reverts commit c87f654.
SUMMARY
This PR enables the new create dataset flow in dataset view, the
+ -> Data
menu in the app header, chart creation, and database creation. On the create dataset page (/dataset/add/
), user is brought to their previous page when the cancel button is clicked and the create chart view when the create dataset button is clicked.TESTING INSTRUCTIONS
/dataset/add
+ Dataset
button in dataset view, the+ -> Data
menu in the app header, chart creation, and database creation.ADDITIONAL INFORMATION