Skip to content
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

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

Merged
merged 8 commits into from
May 2, 2022

Conversation

prosdev0107
Copy link
Contributor

@prosdev0107 prosdev0107 commented Mar 1, 2022

SUMMARY

Hide Tooltip when Column Header Isn't Truncated

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
Always display the Tooltip on the column header
Screen Shot 2022-04-18 at 3 38 54 PM

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

TESTING INSTRUCTIONS

How to reproduce issues

  1. In SQL Lab - run a query with some short and some long column names
  2. 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

@codecov
Copy link

codecov bot commented Mar 1, 2022

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.

@michael-s-molina michael-s-molina requested review from michael-s-molina and removed request for michael-s-molina March 3, 2022 20:00
@rusackas
Copy link
Member

rusackas commented Mar 9, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

@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
Copy link
Member

rusackas commented Mar 9, 2022

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

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

image

@@ -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;
Copy link
Contributor

@diegomedina248 diegomedina248 Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegomedina248 Evan's correct handle is @rusackas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-s-molina crap 🤦 , thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙃

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 18, 2022
@prosdev0107
Copy link
Contributor Author

@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.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 18, 2022
<SortIndicator sortDirection={this.state.sortDirection} />
)}
</div>
// <Tooltip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete commentes :)

);
};

export default TooltipParagraph;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rusackas
Copy link
Member

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

@rusackas rusackas added the need:tests This PR requires tests label Apr 29, 2022
@prosdev0107
Copy link
Contributor Author

@evans
I fixed the merge conflict

@rusackas
Copy link
Member

rusackas commented May 2, 2022

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

I'll review/merge asap.

@rusackas rusackas merged commit 741033e into apache:master May 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2022

Ephemeral environment shutdown and build artifacts deleted.

hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
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
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
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
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:tests This PR requires tests size/L 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants