You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "fisjac (via GitHub)" <gi...@apache.org> on 2023/08/28 20:56:45 UTC
[GitHub] [superset] fisjac opened a new pull request, #25106: fix: preventing save button from flickering in sqlLab
fisjac opened a new pull request, #25106:
URL: https://github.com/apache/superset/pull/25106
…atabase
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Preventing save button from rendering prior to evaluating canExploreDataset to prevent the format from flickering.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25106:
URL: https://github.com/apache/superset/pull/25106#discussion_r1313323397
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -98,6 +98,8 @@ const SaveQuery = ({
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
const canExploreDatabase = !!database?.allows_virtual_table_explore;
+ const canShowSaveButton =
+ database?.allows_virtual_table_explore !== undefined;
Review Comment:
what should the value of canShowSaveButton be if database is null or database.allows_virtual_table_explore is False?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #25106:
URL: https://github.com/apache/superset/pull/25106#issuecomment-1734381418
We need to merge https://github.com/apache/superset/pull/25399/ and rebase this PR so the tests can pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a diff in pull request #25106: fix: preventing save button from flickering in sqlLab
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25106:
URL: https://github.com/apache/superset/pull/25106#discussion_r1308683024
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -97,7 +97,7 @@ const SaveQuery = ({
const [showSave, setShowSave] = useState<boolean>(false);
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
- const canExploreDatabase = !!database?.allows_virtual_table_explore;
+ const canExploreDatabase = database?.allows_virtual_table_explore ? true : null ;
Review Comment:
```suggestion
const canExploreDatabase = database?.allows_virtual_table_explore ? true : false ;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] fisjac commented on a diff in pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on code in PR #25106:
URL: https://github.com/apache/superset/pull/25106#discussion_r1316397497
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -98,6 +98,8 @@ const SaveQuery = ({
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
const canExploreDatabase = !!database?.allows_virtual_table_explore;
+ const canShowSaveButton =
+ database?.allows_virtual_table_explore !== undefined;
Review Comment:
database should always be set since the parent component <SqlEditor/> returns an <EmptyStateBig/> component if database is not defined.
The allows_virtual_table_explore property is added to the store when dispatch(getDatabases) action is called inside of the <SqlEditorLeftTab/>, and then the <SaveQuery/> component gets rerendered with databases.allows_virtual_table_explore as a boolean.
While the getDatabases action is awaiting a response databases.allows_virtual_table_explore will be undefined, and canShowSaveButton prevents the save button from being rendered. Once the additional databases data has been added to the redux store, the save button will either render a split button, or the original button based upon the value canExploreDatabase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] fisjac commented on pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on PR #25106:
URL: https://github.com/apache/superset/pull/25106#issuecomment-1735900234
@betodealmeida I've merged and rebased this PR to the latest from master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25106:
URL: https://github.com/apache/superset/pull/25106#discussion_r1313325950
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -98,6 +98,8 @@ const SaveQuery = ({
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
const canExploreDatabase = !!database?.allows_virtual_table_explore;
+ const canShowSaveButton =
+ database?.allows_virtual_table_explore !== undefined;
Review Comment:
Basically, what's the difference between canExploreDatabase and canShowSaveButton? Also, just a naming nit, I would change canShowSaveButton to something like shouldShowSaveButton, to signify that 1) it's a boolean like you have it, but also 2) that it's not related to permissions but rather state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25106:
URL: https://github.com/apache/superset/pull/25106#discussion_r1336179351
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -97,7 +97,7 @@ const SaveQuery = ({
const [showSave, setShowSave] = useState<boolean>(false);
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
- const canExploreDatabase = !!database?.allows_virtual_table_explore;
+ const canExploreDatabase = database?.allows_virtual_table_explore ? true : null ;
Review Comment:
Better to just cast to boolean:
```js
const canExploreDatabase = !!database?.allows_virtual_table_explore;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida merged pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #25106:
URL: https://github.com/apache/superset/pull/25106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #25106: fix: preventing save button from flickering in SQL Lab
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25106:
URL: https://github.com/apache/superset/pull/25106#issuecomment-1697812465
@fisjac thanks for the change. Would you mind adding a more descriptive PR description as well as a before and after screenshot? This would significantly help the review process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org