-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(explore): SQL popover in datasource panel #19308
Conversation
c05cae5
to
bf5461d
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.
Codes LGTM! Thanks for bringing this Popover, and the SQL is more readable!
There are some no-blocking design suggestions:
- Could we add a scalable modal so that zoom in and out.
- The new icon for SQL expression doesn't look like SQL instead of a calculator.
cc: @kasiazjc
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.
Lovely improvement! LGTM with a few non-blocking comments
superset-frontend/packages/superset-ui-chart-controls/package.json
Outdated
Show resolved
Hide resolved
"@emotion/react": "^11.4.1", | ||
"@superset-ui/core": "*", | ||
"@types/enzyme": "^3.10.5", |
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.
bycatch nit: not yours, but could this be in devDependencies
? Same for @types/react
bf5461d
to
71cc07d
Compare
71cc07d
to
14519aa
Compare
/testenv up |
@kgabryje Ephemeral environment spinning up at http://35.87.97.214:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #19308 +/- ##
=======================================
Coverage 66.58% 66.58%
=======================================
Files 1677 1678 +1
Lines 64238 64242 +4
Branches 6538 6538
=======================================
+ Hits 42773 42777 +4
Misses 19766 19766
Partials 1699 1699
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Ephemeral environment shutdown and build artifacts deleted. |
* feat(explore): SQL popover in datasource panel * Fix acequire not defined * Rebase and fix tests * Disable highlighting gutter * Use ace-build acequire instead of brace
SUMMARY
Previously, we displayed metric's or calculateed column's SQL expression in a tooltip. This PR refactors that component to use a popover and present the SQL in ace editor - thanks to that the code is nicely formatted and easier to read. Also, the icon has been change from a question mark to a calculator.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CC @kasiazjc