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: native filter truncation rerendering loop on hover #20021

Merged
merged 1 commit into from
May 11, 2022

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

This error was detected on a particular dashboard, for a particular set of conditions and only in chrome.

The truncation, in this case, refers to the right content of the SCOPE row.
Screen Shot 2022-05-11 at 01 12 00

The useTruncate function computes the truncated elements by trying to fit one element at a time in the with of their container.
Problem is, by using this information to truncate the element, and add the +X next to it, we're changing the container size, thus triggering another pass at this function.
This, in 99.9% of the cases is fine, but, in very subtle conditions, it can change the truncated count back and forth, if:

  • the size taken by the +X removes the space needed by the last visible element by 1
  • the subtle size gain by the +(X - 1) tag makes enough room for the removed element to appear again.

In this dashboard, the change was between +10 and +11, which has a very different pixel difference, but enough to cause the re-rendering loop.

This PR ensures we only compute the truncated elements if their size changes, or if the size of the parent of the container changes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

For the reasons specified above, and given that the dashboard has private data, no gif is available.
Suffice to say, no change should be visible, the popup and truncation should remain the same for every other use case.

TESTING INSTRUCTIONS

Ensure that the native filters popup content displays correctly, and that the truncation still works as expected.

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 May 11, 2022

Codecov Report

Merging #20021 (5e57203) into master (b7cff83) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master   #20021      +/-   ##
==========================================
- Coverage   66.28%   66.28%   -0.01%     
==========================================
  Files        1712     1712              
  Lines       63968    63985      +17     
  Branches     6731     6734       +3     
==========================================
+ Hits        42404    42414      +10     
- Misses      19853    19860       +7     
  Partials     1711     1711              
Flag Coverage Δ
javascript 51.27% <85.71%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...mponents/nativeFilters/FilterCard/useTruncation.ts 50.00% <85.71%> (+6.52%) ⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js 60.39% <0.00%> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx 32.69% <0.00%> (+0.18%) ⬆️
...et-frontend/src/explore/reducers/exploreReducer.js 28.75% <0.00%> (+1.98%) ⬆️

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 b7cff83...5e57203. Read the comment docs.

@yousoph
Copy link
Member

yousoph commented May 11, 2022

/testenv up

@github-actions
Copy link
Contributor

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

@villebro
Copy link
Member

Thanks for looking into this @diegomedina248 ! I ran into this a few times when testing something unrelated, and wondered if I did something weird that caused it.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Awesome fix and even better explanation! LGTM

@kgabryje kgabryje merged commit cede148 into apache:master May 11, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
@sadpandajoe
Copy link
Contributor

🏷️ preset:2022.19

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request May 11, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@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 preset:2022.19 Preset-Patch size/M 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants