-
Notifications
You must be signed in to change notification settings - Fork 152
minor: adds strategy prop to use when positioning the floating element in popover #1114
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3deefee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
d8bef2a
to
0424d41
Compare
I have read the CLA Document and I hereby sign the CLA |
recheck |
0424d41
to
c7f2213
Compare
@shairez Hey Shai 👋 I hope you're well. It would nice if you could review this tiny PR whenever you have some free time. |
commit: |
thanks @ignatvilesov ! @thejackshelton what is your opinion about this change? |
Positioned elements around an anchor should not be fixed. They are purposely absolute, making them fixed would break a lot of existing apps. To see this in practice, you can edit the css locally in the browser on the qwik ui popover docs from absolute to fixed. That said, position fixed and
Hey @ignatvilesov! Appreciate the initiative on a PR. Have you tried making the outside containing block position relative? It looks like your tooltip does not have a relative container, absolute as a result is going to the bottom of the closest containing block (with the viewport being the last containing block, this goes into the layout algorithms of positioning). Floating UI recommends the absolute strategy as that is the recommended and performant default. I'd be open to exposing an option to having the ability to change strategies and documenting it if there was a clear and common use case for fixed, but if it is just an issue with containing blocks, then that is a behavior of CSS itself. Hope this helps in figuring out the containing block issue 🙏 . In either case as a last resort you can override the styles in your project, as that is the purpose of the styles in an |
Hey @thejackshelton 👋 Thanks for sharing your thoughts and suggestions. Based on my research (tried different things, including relative container), absolute positioning works well in more or less modern browsers. However, it does not work in legacy iOS 15.4 (unfortunately, this version is still in use by our customers) and causes different visual artifacts when applying custom exit animation 😞 Since Please let me know if you have a different opinion 😉 |
c7f2213
to
38afc1e
Compare
Hey @thejackshelton 👋 Just updated the PR and added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, tests continue to pass and functionality remains intact. Instead of three separate files let's have one file called:
popover-types.ts
which includes the context types, floating types and placement types.
Good to merge once this is in.
38afc1e
to
3deefee
Compare
@thejackshelton Thanks for suggestion. Just updated the PR. Let me know if there's anything else to improve. |
What is it?
Why is it needed?
The
strategy
to use when positioning the floating element. The default value isabsolute
, which suites most cases, whilefixed
position might be better in legacy browsers like iOS 15.4.Before
After
Checklist:
pnpm change
and documented my changes