-
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: update dataset editor modal #10347
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10347 +/- ##
==========================================
- Coverage 70.25% 69.83% -0.42%
==========================================
Files 605 196 -409
Lines 32377 19070 -13307
Branches 3271 0 -3271
==========================================
- Hits 22745 13317 -9428
+ Misses 9522 5753 -3769
+ Partials 110 0 -110
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
isDruid: props.datasource.type === 'druid', | ||
isSqla: props.datasource.type === 'table', | ||
isDruid: | ||
props.datasource.type === 'druid' || props.datasource.type === 'druid', |
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 like a redundant check
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 LGTM, just 1 comment
@lilykuang Looks like cypress is failing on this test: https://github.com/preset-io/incubator-superset/blob/3445710267cef7cbbee28b2a49785723fc6d3247/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts#L34 It looks like something that could have been broken with this change |
@@ -0,0 +1,161 @@ | |||
/** |
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.
Not a huge deal, but let's use git mv
to keep the history of the file attached to it! You can fix it quickly with
mv superset-frontend/src/datasource/ChangeDatasourceModal.tsx /tmp
git checkout HEAD^ superset-frontend/src/datasource/ChangeDatasourceModal.tsx
git mv superset-frontend/src/datasource/ChangeDatasourceModal.jsx superset-frontend/src/datasource/ChangeDatasourceModal.tsx
mv /tmp/ChangeDatasourceModal.tsx superset-frontend/src/datasource/
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 couldn't get moving history working for git
012ecd9
to
29f138e
Compare
|
SUMMARY
ChangeDatasourceModal
andDatasourceModal
to use hooksFuture improvement:
DatasourceEditor
to use hooksBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION