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: Adds the DropdownContainer component #21974

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

michael-s-molina
Copy link
Member

SUMMARY

Adds a new component called DropdownContainer. The component is a horizontal container that displays overflowed items in a popover. It also displays an indicator of how many items are currently overflowing.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-10-29.at.6.30.15.PM.mov

TESTING INSTRUCTIONS

Open the Storybook and check the new component.

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

@michael-s-molina
Copy link
Member Author

I already have RTL tests locally but some are still failing. I'll add them soon. I'll also add an MDX file with an overview of the component.

@kgabryje
Copy link
Member

Awesome job! 1 suggestion - in the recording we can see some flashing of appearing/disappearing elements during resizing. I'm assuming that happens when container size is on the verge of fitting or not an additional element. Could we maybe add a few pixels hysteresis to changing the state of displayed/hidden elements?

@michael-s-molina michael-s-molina marked this pull request as ready for review October 31, 2022 16:57
@michael-s-molina
Copy link
Member Author

in the recording we can see some flashing of appearing/disappearing elements during resizing. I'm assuming that happens when container size is on the verge of fitting or not an additional element. Could we maybe add a few pixels hysteresis to changing the state of displayed/hidden elements?

Actually, this is somewhat related to Select's horizontal label. If you set the label position at the top or remove it, you'll see it's no longer flicking. I'll check this alongside the RTL tests. I just wanted to resolve the blocking ones first in case we need to merge the PR to unblock other things.

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #21974 (d3f6349) into master (779d9f7) will decrease coverage by 0.04%.
The diff coverage is 17.18%.

@@            Coverage Diff             @@
##           master   #21974      +/-   ##
==========================================
- Coverage   66.95%   66.91%   -0.05%     
==========================================
  Files        1807     1808       +1     
  Lines       69196    69282      +86     
  Branches     7402     7432      +30     
==========================================
+ Hits        46331    46357      +26     
- Misses      20954    21012      +58     
- Partials     1911     1913       +2     
Flag Coverage Δ
javascript 53.33% <17.18%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...rontend/src/components/DropdownContainer/index.tsx 0.00% <0.00%> (ø)
superset-frontend/src/components/Select/styles.tsx 81.81% <63.63%> (-18.19%) ⬇️
...set-frontend/src/components/Select/AsyncSelect.tsx 87.84% <100.00%> (+0.13%) ⬆️
superset-frontend/src/components/Select/Select.tsx 92.70% <100.00%> (+0.15%) ⬆️
...ashboard/components/BuilderComponentPane/index.tsx 66.66% <0.00%> (-9.81%) ⬇️
...ters/FiltersConfigModal/FiltersConfigForm/utils.ts 77.77% <0.00%> (-4.58%) ⬇️
...t-frontend/src/dashboard/components/SliceAdder.jsx 60.27% <0.00%> (-2.40%) ⬇️
...frontend/src/dashboard/components/Header/index.jsx 58.99% <0.00%> (-0.87%) ⬇️
...et-frontend/src/explore/reducers/exploreReducer.js 38.09% <0.00%> (-0.62%) ⬇️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 52.27% <0.00%> (-0.61%) ⬇️
... and 36 more

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

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.

Tested locally. LGreaTM!

@codyml
Copy link
Member

codyml commented Nov 1, 2022

@michael-s-molina This looks great, thanks! Would it be possible to add a prop for customizing the value in the badge? For horizontal filter bar, I believe we'll need to show the number of active filters, not the total number of filters. If there was a badge count prop, we could handle calls to onOverflowingStateChange by checking which overflowing filters in the dropdown are active and setting the badge accordingly.

@codyml
Copy link
Member

codyml commented Nov 1, 2022

@michael-s-molina For the horizontal filter bar, I think we may also need a way to programmatically open/close the dropdown. Would that be possible to include here, maybe with a useImperativeHandle? I think we'll also want to be able to scroll to and highlight overflow content, so could we also maybe expose a ref to the dropdown content container?

@kgabryje kgabryje merged commit 97e3e79 into apache:master Nov 3, 2022
@kgabryje
Copy link
Member

kgabryje commented Nov 3, 2022

Merged to unblock other tasks, we'll address @codyml's comments in other PRs

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

None yet

5 participants