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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
May 2, 2022
Merged

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

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a4bd893
fix(column-header-tooltip): make that hide the tooltip when the cloum…
prosdev0107 Mar 1, 2022
61e16a0
fix(column-header-tooltip): fix lint
prosdev0107 Mar 1, 2022
f2fa480
fix(column-header-tooltip): make to dynamic tooltip header in FilterT…
prosdev0107 Apr 18, 2022
31be89c
fix(column-header-tooltip): make to fix the lint issue
prosdev0107 Apr 18, 2022
763dd7a
fix(column-header-tooltip): make to remove the tooltip option
prosdev0107 Apr 19, 2022
c830b29
fix(column-header-tooltip): make to add test and storybook for dynami…
prosdev0107 Apr 19, 2022
1683101
fix(column-header-tooltip): make to fix lint
prosdev0107 Apr 19, 2022
9150313
fix(dynamic-tooltip): make to fix merge conflict
prosdev0107 Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 19 additions & 2 deletions superset-frontend/src/components/FilterableTable/FilterableTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ import {
SortIndicator,
Table,
} from 'react-virtualized';
import { getMultipleTextDimensions, t, styled } from '@superset-ui/core';
import {
getTextDimension,
getMultipleTextDimensions,
t,
styled,
} from '@superset-ui/core';
import { Tooltip } from 'src/components/Tooltip';
import Button from '../Button';
import CopyToClipboard from '../CopyToClipboard';
Expand Down Expand Up @@ -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

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 🙃


type CellDataType = string | number | null;
type Datum = Record<string, CellDataType>;
Expand Down Expand Up @@ -213,13 +219,15 @@ export default class FilterableTable extends PureComponent<
// we can't use Math.max(...colWidths.slice(...)) here since the number
// of elements might be bigger than the number of allowed arguments in a
// Javascript function
widthsByColumnKey[key] =
const value =
colWidths
.slice(
index * (this.list.length + 1),
(index + 1) * (this.list.length + 1),
)
.reduce((a, b) => Math.max(a, b)) + PADDING;
widthsByColumnKey[key] =
value > MAX_COLUMN_WIDTH ? MAX_COLUMN_WIDTH : value;
});

return widthsByColumnKey;
Expand Down Expand Up @@ -413,10 +421,19 @@ export default class FilterableTable extends PureComponent<
this.props.expandedColumns.indexOf(label) > -1
? 'header-style-disabled'
: 'header-style';

let trigger: string[] = [];
if (
this.widthsForColumnsByKey[dataKey] <
getTextDimension({ style: { fontSize: '12px' }, text: label }).width
) {
trigger = ['hover'];
}
return (
<Tooltip
id="header-tooltip"
title={label}
trigger={trigger}
placement="topLeft"
css={{ display: 'block' }}
>
Expand Down