[select] Fix support for max-height popup style when alignItemWithTrigger is active#3573
Conversation
atomiks
added
component: select
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: atomiks <cc.glows@gmail.com>
github-actions
Bot
added
the
PR: out-of-date
Signed-off-by: atomiks <cc.glows@gmail.com>
github-actions
Bot
removed
the
PR: out-of-date
| const positionerStyles = getComputedStyle(positionerElement); | ||
| const marginTop = parseFloat(positionerStyles.marginTop); | ||
| const marginBottom = parseFloat(positionerStyles.marginBottom); | ||
| const maxPopupHeight = getMaxPopupHeight(popupRef.current); |
There was a problem hiding this comment.
This calls getComputedStyle every frame when scrolling. How about calling this when scroll starts, caching the value and clearing the cache onScrollEnd?
There was a problem hiding this comment.
That was already being done with positionerStyles though. Also Floating UI does the same when scrolling - I don't think it's a concern
There was a problem hiding this comment.
It's not really a concern, just an opportunity to save some CPU cycles.
There was a problem hiding this comment.
Alright - this can be done in a separate PR as it was already the case in the code previously for the positioner element
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
Outside of the mentioned improvement concern, LGTM. 👍
4 participants
Built off #3532. Specifying
max-heighton.Popupis now respected without the popup moving while scrolling.https://stackblitz.com/edit/dzf17jwn
Fixes #3396