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

only render portal when the element is actually needed to be displayed #2589

Closed
wants to merge 3 commits into from

Conversation

Kiarokh
Copy link
Contributor

@Kiarokh Kiarokh commented Nov 6, 2023

fix https://github.com/Lundalogik/crm-feature/issues/3703
fix #2583

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

Copy link

github-actions bot commented Nov 6, 2023

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-2589/

@Kiarokh Kiarokh changed the title refactor(tooltip): only render portal when tooltip is displayed only render portal when the element is actually needed to be displayed Nov 6, 2023
@john-traas
Copy link
Contributor

john-traas commented Nov 9, 2023

After investigating the side effects of these proposed changes, it seems to unlock potential issues and bugs that could have wide reaching effects.

Some of the potential bugs discovered are potential infinite-loops and intersection observers that are expecting to see an element, which is now not present in the DOM (since with these changes we would hold off rendering the portal if the element is not open).

Much as it might be inefficient to have so many instances of the portal open, it is the underlying structure on which much of the web client is currently built.

With these proposed changes, errors occur in other components specifically with the intersection observer. I think we're opening a potential rabbit hole here and I don't believe the rewards are worth the risk.

@john-traas john-traas closed this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input field: the empty auto suggestion list doesn't really dissapear
2 participants