-
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(chart-controls): Show detailed data type tooltip when hovering type icon #23970
Conversation
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.
@mistercrunch @junlincc Can you please review and approve. Thanks! |
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.
Code LGTM, though tests would be greatly appreciated. Approving to unblock, but pinging others @kgabryje and @michael-s-molina for an additional review in case we're preferring other techniques/components, and @kasiazjc in case there's anything that needs to be adjusted from the UI/design stance.
Thanks @rusackas ! I will add some tests soon. Also, I have applied prettier linting issue fixes. |
@rusackas I have added test cases for my changes and all are passing. Attaching pic for reference. |
@rusackas I have run |
@rusackas All checks have passed now. I have verified all UI behaviors in dev from my end. Please let me know in case anything else is required. Thank you for prompt response! |
Thank you @ved-kashyap-samsung for working on this! I have one suggestion (if possible) can you centre the caret/arrow from the tooltip so that it points to the centre to the icon? With current implementation the caret seems not to be anchored to anything specific and it can be confusing, I think |
@kasiazjc , @rusackas Now, I have made changes to align the tooltip arrow to the center of the icon in this PR. Please check some sample pics from the modified version. Thanks! |
superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #23970 +/- ##
==========================================
+ Coverage 67.71% 68.13% +0.41%
==========================================
Files 1918 1941 +23
Lines 74157 75310 +1153
Branches 8053 8167 +114
==========================================
+ Hits 50219 51314 +1095
- Misses 21885 21907 +22
- Partials 2053 2089 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 156 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx
Outdated
Show resolved
Hide resolved
Really appreciate this fantastic PR, particularly the quick turnaround on suggestions (both code and design), and clear communication. You even added tests, and managed to make CI happy! This is a very well-done first PR, and I hope we see more like it in the future :) I added a couple more (non-blocking) suggestions, but I'm happy to approve it as-is, and look forward to merging this! |
Improved conditional statement
Thanks @rusackas ! Sure, I am looking forward to contribute more and more to superset community in future. Also, thank you for suggesting the changes, I have included your review comments in my latest commit. |
SUMMARY
Fix for issue - #13248
Minor improvement: It show the type as in
VARCHAR(50), text, double precision
when hovering the icon in the metadata panel Please refer the after screenshots below for more understanding.AFTER SCREENSHOTS
TESTING INSTRUCTIONS
Please verify changes from my fork
abc, #
etc.) of column type, it should open a tooltip with detailed data type of that particular columnADDITIONAL INFORMATION