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

Implement scroll into view for popup components #54

Merged
merged 6 commits into from
Apr 27, 2024

Conversation

panoukos41
Copy link
Contributor

As discussed in #52 this implements ScrollIntoView in a generic way in AriaPopupExtensions

Implementation

Since each item has an Id it worked for ListBox and Menu by just passing the JSRuntime to the appropriate modified method (I made it optional but maybe a flag would be appropriate).

For Popover I think we would need more changes to provide access to Ids etc. We could also make the extension public for easier access to developers to customise it as they see fit.

The discarding of the task is intentional since the method is already void and at the same time it should not interupt the ui loop if something goes wrong like not finding the element if it was removed from the dom.

To Discuss

While using it I noticed the following:
As you go up or down sometimes (probably when scrolling stops) the mouseenter event fires as the mouse enters a new li item producing the following effect (as you go up/down the element below the mouse is selected)
msedge_15

Reading around some answers in SO I came accross a solution that suggests to use the mousemove event as the mouseenter works as expected (the mouse entered a new element) but we don't really want it to fire during keyboard scrolling. Here is the result with the mousemove event which does fix the problem:
msedge_17

Should I commit the change for the mouseenter too?

@DavidVollmers
Copy link
Owner

I like the concept! I think it is an easy solution for the problem. I added some comments regarding implementation details and conventions.

As for the mousemove event the problem here is that this would fire a lot of events which will impact performance when the component is server side rendered. Instead we might want to disable the mouseenter logic while scrolling, although that might be difficult to implement.

@panoukos41 panoukos41 marked this pull request as ready for review April 24, 2024 11:44
@panoukos41
Copy link
Contributor Author

I can't see any comments, was it somewhere else?

@DavidVollmers
Copy link
Owner

I had to complete the review, my bad.

@panoukos41
Copy link
Contributor Author

If anything else needs to be resolved let me know and thanks for bearing with me 😅

@DavidVollmers
Copy link
Owner

Looks good to me! I will check it out locally today or latest tomorrow and test a few things between web assembly/server mode but if there are no more concerns I will merge it.

Thank you very much for your contribution so far!

@DavidVollmers DavidVollmers merged commit 7e3eed5 into DavidVollmers:master Apr 27, 2024
4 checks passed
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.

None yet

2 participants