-
Notifications
You must be signed in to change notification settings - Fork 42
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
RichTooltip/Popover
component - hds-anchored-position
modifier [01]
#2020
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
6ce1166
to
043b14c
Compare
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.
Added a few comments/questions
08fa59a
to
bdcc4c3
Compare
RichTooltip/Popover
component - hds-float-popover
modifier [01]RichTooltip/Popover
component - hds-anchored-position
modifier [01]
bdcc4c3
to
3b89e87
Compare
3b89e87
to
854ab58
Compare
854ab58
to
2d1a216
Compare
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.
Reviewed the code and it looks/works well!
It makes sense to have the arrow-related settings grouped, but flattening it makes the in-line modifier call much simpler as it does require an extra hash within the call (e.g. {{hds-anchored-position '#anchor' arrowSelector='#arrow' }}
instead of {{hds-anchored-position '#anchor' arrowOptions=(hash selector='#arrow') }}
)
00c0267
to
d66afdc
Compare
Decided to merge this directly to |
📌 Summary
This PR is part of a larger set of PRs: see #2016
It's mainly a porting (with cleanup/refactoring) of #1947
This PR introduces a modifier built around the Floating UI library, to provide:
For details about why we opted for Floating UI instead of Popper.js refer to this RFC document: https://docs.google.com/document/d/1ey8LVQn0sA0bPbKoaz93ZoYVO1cLcbXUeg1o-tf6LIY/edit
🛠️ Detailed description
In this PR I have:
hds-anchored-position
modifier whose responsibility is to:options
provided to the modifier and create a list of Floating UI middleware functions to invokecomputePosition
main functionautoUpdate
functionoptions
are correctly read and processed by the underlying code logicNotice: while initially the modifier was called
hds-float-popover
and contained references to the "toggle" and "popover" elements, on a second thought we decided to rename ithds-anchored-position
(inspired by this) and generalize code to be more agnostic and remove all the references to "trigger" and “popover” (the modifier itself doesn't know if these concepts, which are related to the "popover" entity)🔗 External links
Jira ticket: https://hashicorp.atlassian.net/browse/HDS-3211 (Main task)
RFC document: https://docs.google.com/document/d/1ey8LVQn0sA0bPbKoaz93ZoYVO1cLcbXUeg1o-tf6LIY/
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.