Skip to content

Fix positioning error#57

Merged
iansan5653 merged 5 commits intomainfrom
fix-positioning-error
May 28, 2024
Merged

Fix positioning error#57
iansan5653 merged 5 commits intomainfrom
fix-positioning-error

Conversation

@iansan5653
Copy link
Member

@iansan5653 iansan5653 commented May 22, 2024 •
edited
Loading

Fixes a bug where the menu position would be calculated incorrectly because dom-input-range gives absolute exact positions and we were still offsetting by the position/scroll of the input.

This approach calculates where the menu is and where it isn't, then adjusts the top and left values based on the difference between the two. This relative approach is a little more complicated than just setting top and left directly but avoids have to make any special consideration for fixed/relative/absolute/top-layer positioning. It's also still very fast as all the computations involved are fairly cheap.

Also adds a floating menu to the demo so we can see this in action.

@iansan5653 iansan5653 requested a review from a team as a code owner May 22, 2024 20:45
@keithamus
Copy link
Contributor

keithamus commented May 23, 2024

Does this account for scroll containers? It's dropping the scrollx code which tells me it might not work properly if scrolled.

@iansan5653
Copy link
Member Author

iansan5653 commented May 23, 2024 •
edited
Loading

Sorry, didn't intend to mark this ready for review yet. I need to figure out a way to test this locally in dotcom.

Does this account for scroll containers? It's dropping the scrollx code which tells me it might not work properly if # scrolled.

Theoretically scrolling should work as expected: DomInputRange is already accounting for scroll. When getBoundingClientRect is called on the range, the result is exactly where we want the menu to be relative to the document, so it's just a matter of making sure that's where the menu ends up.

@iansan5653 iansan5653 marked this pull request as draft May 23, 2024 14:19
@iansan5653 iansan5653 force-pushed the fix-positioning-error branch from e7948d0 to 912db2b Compare May 23, 2024 19:28
@iansan5653 iansan5653 marked this pull request as ready for review May 23, 2024 19:30
@iansan5653
Copy link
Member Author

iansan5653 commented May 23, 2024

Okay, now we're ready for review. Tested in dotcom by manually copying in the changes and it's working great. I updated the approach to only have to set top and left once (fewer repaints!) and extracted a positionMenu function.

keithamus
keithamus approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@silverwind
Copy link
Contributor

silverwind commented May 27, 2024 •
edited
Loading

Also noticed that the positioning in @github/text-expander-element@2.7.0 is rather broken compared to 2.6.1, e.g. too far in both the x and y axis. I hope this fixes that.

image

@iansan5653
Copy link
Member Author

iansan5653 commented May 28, 2024

Also noticed that the positioning in @github/text-expander-element@2.7.0 is rather broken compared to 2.6.1, e.g. too far in both the x and y axis. I hope this fixes that.

Yes, this should fix that! Will merge today and release as 2.7.1; please file a bug if you continue to see problems after that release

@iansan5653 iansan5653 merged commit fc7f9b6 into main May 28, 2024
@silverwind
Copy link
Contributor

silverwind commented May 28, 2024

Thanks, I can confirm 2.7.1 fixed that issue for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@keithamus keithamus keithamus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iansan5653 @keithamus @silverwind