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

feat(native-filters): Adjust filter components for horizontal mode #22273

Merged

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Nov 30, 2022

SUMMARY

This PR adjusts native filter components for horizontal filter bar.
Noteworthy changes:

  1. Value filter:
    a) Introduce a oneLine mode to Select component - if enabled, only 1 tag will be displayed at once and the rest of selected tags will be collapsed to an overflow tag. When user opens the dropdown, only the overflow tag is displayed. All tags are always displayed in a single line.
    b) If tag is truncated, display a tooltip with full name
    c) Customize maxTagPlaceholder, so that the overflow tag doesn't display 3 dots at the end, which takes precious space
    (e.g. instead of the default + 1 ... we show +1 when 1 item is overflowing)
  2. Range filter:
    a) Adjust the top and bottom margin to make sure that the slider is centered in horizontal mode
    b) Decrease font weight and line height so that the marks fit well in the horizontal filter bar
  3. Time range filter: add styling to make sure that the pill is centered in horizontal mode

Known issues:

  1. The time range pill's width is dynamic depending on selected time range ("No filter" pill is smaller than "2022-01-01 : 2022-12-01" pill). However, the filter component itself always has a fixed width, which means that if the pill is short, there might be some empty space between the time range filter and the filter next to it. We are planning to replace it soon with a new component, stay tuned!

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshot 2022-11-30 at 14 49 21

TESTING INSTRUCTIONS

  1. Enable HORIZONTAL_FILTER_BAR ff
  2. Set horizontal mode for the native filters bar
  3. Add various types of filters
  4. Make sure that nothing breaks

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

CC @codyml for review

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #22273 (4f76d87) into master (2bdf22b) will increase coverage by 0.00%.
The diff coverage is 68.57%.

❗ Current head 4f76d87 differs from pull request most recent head 1d38d63. Consider uploading reports for the commit 1d38d63 to get more accurate results

@@           Coverage Diff           @@
##           master   #22273   +/-   ##
=======================================
  Coverage   55.69%   55.70%           
=======================================
  Files        1846     1847    +1     
  Lines       70302    70330   +28     
  Branches     7689     7701   +12     
=======================================
+ Hits        39157    39177   +20     
- Misses      29151    29155    +4     
- Partials     1994     1998    +4     
Flag Coverage Δ
javascript 53.81% <68.57%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...Filters/FilterBar/FilterControls/FilterControl.tsx 29.03% <0.00%> (ø)
...veFilters/FilterBar/FilterControls/FilterValue.tsx 6.31% <0.00%> (ø)
...end/src/filters/components/Range/transformProps.ts 100.00% <ø> (ø)
...nd/src/filters/components/Select/transformProps.ts 91.66% <ø> (ø)
...et-frontend/src/filters/components/Select/types.ts 100.00% <ø> (ø)
...d/src/filters/components/Time/TimeFilterPlugin.tsx 0.00% <ø> (ø)
...tend/src/filters/components/Time/transformProps.ts 0.00% <ø> (ø)
...src/filters/components/Range/RangeFilterPlugin.tsx 68.80% <40.00%> (-1.95%) ⬇️
...erset-frontend/src/components/Select/CustomTag.tsx 64.28% <64.28%> (ø)
...es/superset-ui-core/src/chart/models/ChartProps.ts 100.00% <100.00%> (ø)
... and 3 more

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

@kgabryje kgabryje changed the title Feat/horizontal native filters feat(native-filters): Adjust filter components for horizontal mode Nov 30, 2022
@geido
Copy link
Member

geido commented Nov 30, 2022

/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true

@github-actions
Copy link
Contributor

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

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

Just two nits from a first look at the code

@kgabryje kgabryje force-pushed the feat/horizontal-native-filters branch from 46808da to 3b00821 Compare December 1, 2022 14:10
@kgabryje kgabryje requested a review from ktmud as a code owner December 1, 2022 14:10
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 1, 2022
@kgabryje
Copy link
Member Author

kgabryje commented Dec 1, 2022

@michael-s-molina @geido @codyml I implemented the Select changes based on our yesterday discussion. Can you take a look?

Copy link
Member

@michael-s-molina michael-s-molina 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 PR @kgabryje. I left a couple of comments.

Comment on lines 56 to 73
${
oneLine &&
`
.ant-select-selection-overflow {
flex-wrap: nowrap;
}

.ant-select-selection-overflow-item:not(.ant-select-selection-overflow-item-rest):not(.ant-select-selection-overflow-item-suffix) {
flex-shrink: 1;
min-width: ${theme.gridUnit * 13}px;
}

.ant-select-selection-overflow-item-suffix {
flex: unset;
min-width: 0px;
}
`
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
${
oneLine &&
`
.ant-select-selection-overflow {
flex-wrap: nowrap;
}
.ant-select-selection-overflow-item:not(.ant-select-selection-overflow-item-rest):not(.ant-select-selection-overflow-item-suffix) {
flex-shrink: 1;
min-width: ${theme.gridUnit * 13}px;
}
.ant-select-selection-overflow-item-suffix {
flex: unset;
min-width: 0px;
}
`
}
${
oneLine &&
`
.ant-select-selection-overflow {
flex-wrap: nowrap;
}
.ant-select-selection-overflow-item:not(.ant-select-selection-overflow-item-rest):not(.ant-select-selection-overflow-item-suffix) {
flex-shrink: 1;
min-width: ${theme.gridUnit * 13}px;
}
.ant-select-selection-overflow-item-suffix {
flex: unset;
min-width: 0px;
}
`
}

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

// TODO: use antd Tag props instead of any. Currently it's causing a typescript error
const Tag = (props: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

It seems the custom render is necessary to make oneLine work correctly so we need to move this code to the Select component.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@kgabryje kgabryje force-pushed the feat/horizontal-native-filters branch from 3b00821 to 7f114aa Compare December 1, 2022 17:06
@geido
Copy link
Member

geido commented Dec 1, 2022

/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

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

@kgabryje
Copy link
Member Author

kgabryje commented Dec 1, 2022

/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

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

Copy link
Member

@codyml codyml left a comment

Choose a reason for hiding this comment

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

LGTM! One thing I noticed that can probably wait for a future PR (or left as-is) is that if there's a time filter in the overflow dropdown, clicking either of the popover buttons also closes the dropdown, which is probably not what we want:

Screen.Recording.2022-12-01.at.1.00.25.PM.mov

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@kgabryje Let's open a follow-up to add the same behavior for the async version 😉

@michael-s-molina
Copy link
Member

LGTM! One thing I noticed that can probably wait for a future PR (or left as-is) is that if there's a time filter in the overflow dropdown, clicking either of the popover buttons also closes the dropdown, which is probably not what we want:

Good point @codyml. Let's handle that in a follow-up.

@michael-s-molina michael-s-molina merged commit eb6045a into apache:master Dec 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants