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 for the ComboBox positioning issue #48

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

kaizjen
Copy link
Contributor

@kaizjen kaizjen commented May 29, 2022

This PR modifies how <ComboBox> calculates its offset.

It uses el.getBoundingClientRect() to detect the difference between the selected item and the ComboBox's button and moves the menu by this difference.

This does not fix the other issue when the ComboBox will be displayed out of the screen's bounds. I left a proposed solution commented out, because I think it's not the most optimal way of doing it.

Doesn't yet close #6, because some display issues still remain.

@vercel
Copy link

vercel bot commented May 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
fluent-svelte ✅ Ready (Inspect) Visit Preview May 29, 2022 at 4:16PM (UTC)

@kaizjen
Copy link
Contributor Author

kaizjen commented May 29, 2022

Note that to compare how my solution works with the current ComboBox implementation, you will need to create more than 14 items in the ComboBox. I didn't want to edit the 'test' page, so I decided not to include it in the commit

@Tropix126
Copy link
Owner

Thank you! LGTM.

@Tropix126 Tropix126 merged commit 6d487dc into Tropix126:main Jun 4, 2022
@kaizjen
Copy link
Contributor Author

kaizjen commented Jun 8, 2022

There's an interesting issue with my implementation: it doesn't work with scroll-behavior: smooth. It seems like it's records the selected item's position before it was actually scrolled to.

To fix this, we just need to put scroll-behavior: auto on the <ul> element. I guess I should make another PR for this, since this one was already merged?

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.

ComboBox display issues
2 participants