feat: Update native filter dividers to support horizontal display #21970

codyml · 2022-10-29T00:14:23Z

SUMMARY

This PR updates native filter dividers to display correctly when native filters are rendered as a horizontal bar, which will be supported in a future PR. In this PR, the divider is factored out into its own component, FilterDivider, which now supports three view modes:

Vertical: Same as current; displays full title and full description, if set.
Horizontal: Displays title, subject to truncation. If title is truncated or if there is a description set, an icon appears with a tooltip showing the full title (if truncated) and/or the description (if set).
Horizontal overflow: Displays title and description, if set, both subject to truncation. Hovering over truncated description or title will show full text.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Vertical (same as before):

Horizontal:

Horizontal overflow:

TESTING INSTRUCTIONS

Check that current native filters dividers look and work the same as they currently do.
Run Storybook locally off of this PR and open the FilterDivider component, which should contain Vertical Filter Divider, Horizontal Filter Divider, and Horizontal Overflow Filter Divider. Check that all three behave as expected with:
- Short title
- Long title
- No description
- Short description
- Long description

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 · 2022-10-29T01:03:26Z

Codecov Report

Merging #21970 (89fde38) into master (72598a5) will increase coverage by 0.01%.
The diff coverage is 71.23%.

❗ Current head 89fde38 differs from pull request most recent head 3d175ff. Consider uploading reports for the commit 3d175ff to get more accurate results

@@            Coverage Diff             @@
##           master   #21970      +/-   ##
==========================================
+ Coverage   65.72%   65.74%   +0.01%     
==========================================
  Files        1809     1811       +2     
  Lines       69335    69370      +35     
  Branches     7413     7425      +12     
==========================================
+ Hits        45571    45606      +35     
  Misses      21853    21853              
  Partials     1911     1911

Flag	Coverage Δ
javascript	`53.45% <71.23%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
...ilters/FilterBar/FilterControls/FilterControls.tsx	`70.58% <ø> (ø)`
...c/hooks/useTruncation/useChildElementTruncation.ts	`44.73% <44.73%> (ø)`
...Filters/FilterBar/FilterControls/FilterDivider.tsx	`100.00% <100.00%> (ø)`
...nd/src/hooks/useTruncation/useCSSTextTruncation.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zhaoyongjie · 2022-10-31T03:56:53Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+  title: string;
+  description: string;
+  horizontal?: boolean;
+  horizontalOverflow?: boolean;


@codyml I'm curious whether the verticalOverflow might be in the future. If it is, should we use props like overflow to control this?

Sounds good, changed horizontalOverflow to overflow in case we want to support that in the future. Since the styles are unique, another option is that I could have an orientation field that accepts constants for each supported orientation (vertical, horizontal, horizontal overflow) and we could add a 4th constant if we end up wanting to support vertical overflow dropdown – let me know if that sounds better.

geido

Thanks for the awesome work! Just a few nits from a first look

geido · 2022-11-01T08:58:56Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+  horizontalOverflow?: boolean;
+}
+
+const useIsTruncated = <T extends HTMLElement>(


I saw this hook being used in at least 3 more places in the app. We should move this to the common hooks if possible. In a separate PR we can then go back and replace it everywhere else

Looks like there's an useTruncation hook already in src/hooks, but it's for the separate use-case of multiple child elements with element truncation controlled by JS, rather than one element with text truncation controlled by CSS. I kept the default export of useTruncation the element version and added a new export of useCSSTextTruncation – does that seem like a good solution?

geido · 2022-11-01T09:00:22Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+          white-space: nowrap;
+          overflow: hidden;
+          text-overflow: ellipsis;
+          font-size: 14px;


We should use the provided typography settings in the theme

geido · 2022-11-01T09:00:39Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+            data-test="divider-description-icon"
+            css={(theme: SupersetTheme) => css`
+              color: ${theme.colors.grayscale.base};
+              font-size: 16px;


Icons have a property iconSize with predefined sizes. We should stick to those whenever possible.

Done! Also moved color to iconColor.

geido · 2022-11-01T09:01:39Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+            text-overflow: ellipsis;
+            color: ${theme.colors.grayscale.dark1};
+            font-weight: normal;
+            font-size: 14px;


Same comment about font-size as above

geido · 2022-11-01T09:01:51Z

...t-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterDivider.tsx

+              white-space: nowrap;
+              overflow: hidden;
+              text-overflow: ellipsis;
+              font-size: 12px;


codyml · 2022-11-19T02:42:21Z

Closing in favor of #22169.

pull-request-size bot added the size/L label Oct 29, 2022

codyml force-pushed the feat/horizontal-divider branch from a91b9e2 to c5a0b56 Compare October 29, 2022 00:56

zhaoyongjie reviewed Oct 31, 2022

View reviewed changes

geido reviewed Nov 1, 2022

View reviewed changes

pull-request-size bot added size/XL and removed size/L labels Nov 1, 2022

codyml added 10 commits November 1, 2022 14:47

Remove unnecessary JSON stringify check of immutable object.

659ef8b

Factor divider into separate component; add tests.

5404ff0

Add storybook, WIP on horizontal divider.

f08485d

Implement truncation w/ tooltip behavior.

e9539d2

Finish unit tests.

b72e80f

Style components.

105ebdd

Replace horizontalOverflow prop name with overflow.

c36ee8f

Factor out truncation hook.

13b9fcc

Use predefined sizes.

a8e230f

Fix missing license.

3d175ff

codyml force-pushed the feat/horizontal-divider branch from 5fadb30 to 3d175ff Compare November 1, 2022 20:48

codyml closed this Nov 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update native filter dividers to support horizontal display #21970

feat: Update native filter dividers to support horizontal display #21970

codyml commented Oct 29, 2022

codecov bot commented Oct 29, 2022 •

edited

Loading

zhaoyongjie Oct 31, 2022 •

edited

Loading

codyml Nov 1, 2022

geido left a comment

geido Nov 1, 2022

codyml Nov 1, 2022 •

edited

Loading

geido Nov 1, 2022

codyml Nov 1, 2022

geido Nov 1, 2022

codyml Nov 1, 2022 •

edited

Loading

geido Nov 1, 2022

geido Nov 1, 2022

codyml commented Nov 19, 2022

feat: Update native filter dividers to support horizontal display #21970

feat: Update native filter dividers to support horizontal display #21970

Conversation

codyml commented Oct 29, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 29, 2022 • edited Loading

Codecov Report

zhaoyongjie Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

codyml Nov 1, 2022

Choose a reason for hiding this comment

geido left a comment

Choose a reason for hiding this comment

geido Nov 1, 2022

Choose a reason for hiding this comment

codyml Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

geido Nov 1, 2022

Choose a reason for hiding this comment

codyml Nov 1, 2022

Choose a reason for hiding this comment

geido Nov 1, 2022

Choose a reason for hiding this comment

codyml Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

geido Nov 1, 2022

Choose a reason for hiding this comment

geido Nov 1, 2022

Choose a reason for hiding this comment

codyml commented Nov 19, 2022

codecov bot commented Oct 29, 2022 •

edited

Loading

zhaoyongjie Oct 31, 2022 •

edited

Loading

codyml Nov 1, 2022 •

edited

Loading

codyml Nov 1, 2022 •

edited

Loading