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(core): overflow controller resize loop #2855

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

zeroedin
Copy link
Collaborator

@zeroedin zeroedin commented Sep 23, 2024

Downstream issue:

❌ > on small screen > reversed right to left language overflow actions > previousTab should be disabled
Error: ResizeObserver loop completed with undelivered notifications. (http://localhost:8003/?wtr-session-id=E_xerQsq1EQq2yny1h-Ga:0)

If you want to prevent these errors, the solution depends on what your intended effect is. If you actually intend to have an infinite loop, you just need to defer the resizing code in your ResizeObserver callback to after the browser repaints. You can put it into a requestAnimationFrame callback.

Documentation Reference

What I did

  1. Await browser repaint before triggering overflow side effects

Testing Instructions

Notes to Reviewers

@zeroedin zeroedin self-assigned this Sep 23, 2024
Copy link

changeset-bot bot commented Sep 23, 2024

🦋 Changeset detected

Latest commit: ac18535

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@patternfly/pfe-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Sep 23, 2024

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 288f7b6
😎 Deploy Preview https://deploy-preview-2855--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the AT passed Automated testing has passed label Sep 23, 2024
@zeroedin zeroedin changed the title Fix/core/overflow controller resize loop fix(core) overflow controller resize loop Sep 23, 2024
@bennypowers bennypowers changed the title fix(core) overflow controller resize loop fix(core): overflow controller resize loop Sep 24, 2024
Copy link
Contributor

github-actions bot commented Sep 24, 2024

✅ Commitlint tests passed!

More Info
{
  "valid": true,
  "errors": [],
  "warnings": [],
  "input": "fix(core): overflow controller resize loop"
}

core/pfe-core/controllers/overflow-controller.ts Outdated Show resolved Hide resolved
.changeset/fluffy-bananas-remember.md Outdated Show resolved Hide resolved
.changeset/fluffy-bananas-remember.md Outdated Show resolved Hide resolved
@bennypowers bennypowers marked this pull request as ready for review September 24, 2024 05:30
zeroedin and others added 3 commits September 24, 2024 09:24
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
@zeroedin
Copy link
Collaborator Author

zeroedin commented Sep 24, 2024

@bennypowers adding await aTimeout(50); allows the pf-tabs tests to complete. That said I'm not convinced that just adding a timeout to the test is the correct way to handle red/green here. Another way to do it is await 3x nextFrame.

beforeEach(nextFrame);
beforeEach(nextFrame);
beforeEach(nextFrame);
beforeEach(updateComplete);

I can almost understand the need for nextFrame better here as we are now adding a tick given the overflow controller implementing the requestAnimationFrame. Thoughts?

@bennypowers bennypowers merged commit 0ec7338 into main Sep 25, 2024
15 of 17 checks passed
@bennypowers bennypowers deleted the fix/core/overflow-controller-resize-loop branch September 25, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants