fix(column-header-tooltip): make that hide the tooltip when the cloum… #18988

prosdev0107 · 2022-03-01T18:31:47Z

SUMMARY

Hide Tooltip when Column Header Isn't Truncated

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
Always display the Tooltip on the column header

AFTER:
Make that hide the Tooltip when the column isn't truncated.

TESTING INSTRUCTIONS

How to reproduce issues

In SQL Lab - run a query with some short and some long column names
In the results table, a tooltip shows on all column headers

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

…n header is turncated

codecov · 2022-03-01T19:22:04Z

Codecov Report

Merging #18988 (e7fb04c) into master (36d45d9) will increase coverage by 0.00%.
The diff coverage is 72.72%.

❗ Current head e7fb04c differs from pull request most recent head 9150313. Consider uploading reports for the commit 9150313 to get more accurate results

@@           Coverage Diff           @@
##           master   #18988   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files        1714     1715    +1     
  Lines       65033    65038    +5     
  Branches     6717     6718    +1     
=======================================
+ Hits        43261    43266    +5     
  Misses      20065    20065           
  Partials     1707     1707

Flag	Coverage Δ
javascript	`51.25% <72.72%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...src/components/FilterableTable/FilterableTable.tsx	`71.59% <57.14%> (+0.16%)`	⬆️
...frontend/src/components/TooltipParagraph/index.tsx	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36d45d9...9150313. Read the comment docs.

rusackas · 2022-03-09T19:47:53Z

/testenv up

github-actions · 2022-03-09T19:49:39Z

@rusackas Ephemeral environment spinning up at http://52.12.207.55:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas · 2022-03-09T20:55:20Z

Testing the PR, it's not clear that the headers are ever truncated.

If I'm mistaken, please add testing steps and/or screenshots.

diegomedina248 · 2022-03-17T03:23:16Z

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

@@ -95,6 +100,7 @@ const StyledFilterableTable = styled.div`

 // when more than MAX_COLUMNS_FOR_TABLE are returned, switch from table to grid view
 export const MAX_COLUMNS_FOR_TABLE = 50;
+export const MAX_COLUMN_WIDTH = 200;


@rusackas was this decided somewhere as a breakpoint?
If not, @prosdev0107 consider using this if possible, which is a dynamic way of doing it.

@diegomedina248 Evan's correct handle is @rusackas

@michael-s-molina crap 🤦 , thanks

THANK YOU. I spent a few minutes trying to find an example of the AntD-native approach. I swear I had a CodePen example of this that I once wrote up, but I seem to have left it in my other coat.

The 200px is indeed arbitrary. It also doesn't seem to matter, thus my comment/screenshot above.

I'm OK with setting a reasonable maximum column width. Exactly how wide is up for grabs. Maybe @kasiazjc has opinions on the matter. We should also test/consider how the contents of the cell are wrapped/truncated when hitting this maximum width.

The only downside to the AntD approach is a little more vendor lock-in, should we ever decide to change component libraries again... but it probably won't be our biggest battle then :)

The getTextDimension approach seems like it'd work, but is slightly fragile-looking to me as a technique to start reusing in more places.

Sorry for the late response, but re: columns setting max/min width can be tricky I think. To do it well we would have to go through different types of data there are in the columns and define min/max... If we would introduce max width for columns and let's say we would have 2 columns it could look weird as the table would not be stretched from left or right.

Easy solution would be to scale the column width, so depending on the width of the table all columns always have the same width 🤔 but still, lot's of edge case.

That is, if I understand the problem correctly 🙃

rusackas

OK, regrouping my thoughts on what's needed for this PR to be ready:
• More details in the description regarding what the problem is, and where it can be seen (I didn't realize we were talking about SQL Lab at first, which led to my feedback sending the PR on a bit of a tangent).
• Probably(?) getting rid of the max-width added... if these column headers were being truncated before, we probably don't need to add an arbitrary max width (sounds like there is one already)
• I'd much prefer to go the AntD official dynamic-tooltip route than the text-dimension-measuring route.
• Before and After screenshots/gifs/movies would really help
• Basic repro/test steps would help.
• RTL tests to make sure it works as intended, and stays that way :)

…able

prosdev0107 · 2022-04-18T19:43:51Z

@rusackas @diegomedina248
I added the BEFORE/AFTER screenshots and repro/test steps into PR.
And also I used Antd dynamic tooltip.

One problem is that the column header is always set as the maximum value between header text length and row content length. Thus, the column header would be not always truncated.

rusackas · 2022-04-18T20:47:05Z

superset-frontend/src/components/FilterableTable/FilterableTable.tsx

+          <SortIndicator sortDirection={this.state.sortDirection} />
+        )}
+      </div>
+      // <Tooltip


Delete commentes :)

rusackas · 2022-04-18T20:47:49Z

superset-frontend/src/components/TooltipParagraph/index.tsx

+  );
+};
+
+export default TooltipParagraph;


If we're going to keep this component, which has value, let's add a storybook entry for it, and at least basic unit/RTL tests.

…c tooltip

rusackas

Thanks for the improvements. We'll follow up with tests in another PR.

rusackas · 2022-04-29T18:30:42Z

@prosdev0107 can you resolve the conflicts? We'll get this merged :)

prosdev0107 · 2022-04-29T19:07:49Z

@evans
I fixed the merge conflict

rusackas · 2022-05-02T16:04:49Z

Please note that my handle is @rusackas. We've gotta stop spamming this other guy :)

I'll review/merge asap.

github-actions · 2022-05-02T16:11:59Z

Ephemeral environment shutdown and build artifacts deleted.

apache#18988) * fix(column-header-tooltip): make that hide the tooltip when the cloumn header is turncated * fix(column-header-tooltip): fix lint * fix(column-header-tooltip): make to dynamic tooltip header in FilterTable * fix(column-header-tooltip): make to fix the lint issue * fix(column-header-tooltip): make to remove the tooltip option * fix(column-header-tooltip): make to add test and storybook for dynamic tooltip * fix(column-header-tooltip): make to fix lint

fix(column-header-tooltip): make that hide the tooltip when the cloum…

a4bd893

…n header is turncated

pull-request-size bot added the size/S label Mar 1, 2022

fix(column-header-tooltip): fix lint

61e16a0

michael-s-molina requested review from michael-s-molina and removed request for michael-s-molina March 3, 2022 20:00

diegomedina248 reviewed Mar 17, 2022

View reviewed changes

rusackas requested changes Mar 25, 2022

View reviewed changes

fix(column-header-tooltip): make to dynamic tooltip header in FilterT…

f2fa480

…able

pull-request-size bot added size/M and removed size/S labels Apr 18, 2022

fix(column-header-tooltip): make to fix the lint issue

31be89c

pull-request-size bot added size/L and removed size/M labels Apr 18, 2022

rusackas reviewed Apr 18, 2022

View reviewed changes

prosdev0107 added 3 commits April 19, 2022 09:47

fix(column-header-tooltip): make to remove the tooltip option

763dd7a

fix(column-header-tooltip): make to add test and storybook for dynami…

c830b29

…c tooltip

fix(column-header-tooltip): make to fix lint

1683101

rusackas approved these changes Apr 29, 2022

View reviewed changes

rusackas added the need:tests This PR requires tests label Apr 29, 2022

fix(dynamic-tooltip): make to fix merge conflict

9150313

rusackas approved these changes May 2, 2022

View reviewed changes

rusackas merged commit 741033e into apache:master May 2, 2022

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(column-header-tooltip): make that hide the tooltip when the cloum… #18988

fix(column-header-tooltip): make that hide the tooltip when the cloum… #18988

prosdev0107 commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

rusackas commented Mar 9, 2022

github-actions bot commented Mar 9, 2022

rusackas commented Mar 9, 2022

diegomedina248 Mar 17, 2022 •

edited

Loading

michael-s-molina Mar 17, 2022

diegomedina248 Mar 17, 2022

rusackas Mar 17, 2022

rusackas Mar 17, 2022

kasiazjc Mar 22, 2022

rusackas left a comment

prosdev0107 commented Apr 18, 2022

rusackas Apr 18, 2022

rusackas Apr 18, 2022

rusackas left a comment

rusackas commented Apr 29, 2022

prosdev0107 commented Apr 29, 2022

rusackas commented May 2, 2022

github-actions bot commented May 2, 2022

fix(column-header-tooltip): make that hide the tooltip when the cloum… #18988

fix(column-header-tooltip): make that hide the tooltip when the cloum… #18988

Conversation

prosdev0107 commented Mar 1, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 1, 2022 • edited Loading

Codecov Report

rusackas commented Mar 9, 2022

github-actions bot commented Mar 9, 2022

rusackas commented Mar 9, 2022

diegomedina248 Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Mar 17, 2022

Choose a reason for hiding this comment

diegomedina248 Mar 17, 2022

Choose a reason for hiding this comment

rusackas Mar 17, 2022

Choose a reason for hiding this comment

rusackas Mar 17, 2022

Choose a reason for hiding this comment

kasiazjc Mar 22, 2022

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

prosdev0107 commented Apr 18, 2022

rusackas Apr 18, 2022

Choose a reason for hiding this comment

rusackas Apr 18, 2022

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

rusackas commented Apr 29, 2022

prosdev0107 commented Apr 29, 2022

rusackas commented May 2, 2022

github-actions bot commented May 2, 2022

prosdev0107 commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

diegomedina248 Mar 17, 2022 •

edited

Loading