Tabs: update indicator more reactively#66207
Conversation
ciampo
added
[Type] Bug
| // Check that the targetElement is still attached to the DOM, in case | ||
| // it was removed since the last `measure` call. | ||
| if ( targetElement && targetElement.isConnected ) { |
There was a problem hiding this comment.
This is a separate fix for a bug that I discovered while working on this PR. Basically, the measure function could go on an infinite loop in case the targetElement was holding a reference to an old node that in the meantime was removed from the DOM. The isConnected check makes sure that we can detect that edge case and act correctly.
An example of this:
- in dev mode, react renders twice for the first render
- this was causing the
instanceIdof the wholeTabsto update, causing also the individualTabs.Tabbuttons to update. - therefore, this
measurefunction would loop indefinitely on first render
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 6794d66. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11386549804
|
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Makes sense and tests well, thanks 👍 🚀
| measure(); | ||
| // `measure` is a stable function, so it's safe to omit it from the deps array. | ||
| // deps can't be statically analyzed by ESLint | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
ESLint disabling is a problem for React Compiler.
I'd prefer if we leave the warning in place. There are plenty of those warnings across the codebase already. Yes, we'll have to deal with them, but that can be separately addressed.
There was a problem hiding this comment.
As discussed, we will address this at a later point, so that we can work across the whole reporsitory
ciampo
force-pushed
the
fix/tabs-removal-indicator-sync
branch
from
6794d66 to
0c27a3f
Compare
Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
2 participants
What?
Fixes #65576
Improve how
Tabsreacts to changes to tabs in the tablist other than resizingWhy?
Fixes a bug
How?
So far, we were re-measuring the indicator position only when the tab's dimensions change (via a `ResizeObserver), but this was not enough. For example, a tab item could be swapped with another one, or one or more tab items could be added/removed to the tablist.
The changes in this PR introduce a way to add a dependency array to the hook in charge of measuring the indicator size, so that we can arbitrarily recalculate. For now,
Tabsis using the index of the selected tab item as the dependency, but in the future we may want to add more as needed (even use aMutationObserver).Testing Instructions
The bug flagged in #65576 should be gone.
Screenshots or screencast
Kapture.2024-10-17.at.16.12.26.mp4
Kapture.2024-10-17.at.16.11.48.mp4