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

Use Element.scrollIntoView #75

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Sep 5, 2023

Related to @github/auto-complete-element#91

The current scrollTo helper method implementation can be unreliable (or have no effect) at times.

This commit replaces it with a call to Element.scrollIntoView. To control that behavior, this commit also introduces a scrollIntoViewOptions: key to the package's constructor call.

@seanpdoyle seanpdoyle requested a review from a team as a code owner September 5, 2023 22:38
Related to [@github/auto-complete-element#91][]

The current `scrollTo` helper method implementation can be unreliable
(or have no effect) at times.

This commit replaces it with a call to [Element.scrollIntoView][]. To
control that behavior, this commit also introduces a
`scrollIntoViewOptions:` key to the package's constructor call.

[@github/auto-complete-element#91]: github/auto-complete-element#91
[Element.scrollIntoView]: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView
[ScrollIntoViewOptions]: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView#sect1
@seanpdoyle
Copy link
Contributor Author

@keithamus I did a preliminary search for attempts to replace the scrollTo implementation with calls to Element.scrollIntoView, but came up empty:

~/Work/github/combobox-nav main
❯ git log -SscrollIntoView

A quick dive into git blame yielded a commit that copied the scrollTo implementation from github/auto-complete-element. A search for scrollIntoView on that project didn't yield any results.

Has this been attempted in the past?

@primer-css
Copy link

👋 Hello and thanks for pinging us! This issue or PR has been added to our inbox and a Primer first responder will review it soon.

  • 🎨 If this is a PR that includes a visual change, please make sure to add screenshots in the description or deploy this code to a lab machine with instructions for how to test.
  • If this is a PR that includes changes to an interaction, please include a video recording in the description.
  • ⚠️ If this is urgent, please visit us in #primer on Slack and tag the first responders listed in the channel topic.

@broccolinisoup
Copy link
Contributor

@keithamus is this something you can take on to answer or do you prefer for me to look as a FR?

@keithamus
Copy link
Member

I’ll take a look thanks @broccolinisoup

@nicowenterodt
Copy link
Contributor

The use of Element.scrollIntoView is really helpful. I checked this and it works wonderful especially if you need to control the scroll position and behavior of it as easy as defining an option like this element.scrollIntoView({ behavior: "smooth", block: "end", inline: "nearest" });

Thanks @seanpdoyle 👏

I would love to see this one merged @keithamus.

@keithamus
Copy link
Member

I agree I just want to check some internal codebases to double check if there are any blockers before we merge. I'm a little short on time this week so I'll most likely get to this next week.

@keithamus
Copy link
Member

macos build failing for some reason. I'll figure that out separately.

@keithamus keithamus merged commit a5337fb into github:main Sep 25, 2023
2 of 3 checks passed
earl-warren pushed a commit to earl-warren/gitea that referenced this pull request Dec 19, 2023
- The v2.3.0 update caused to always scroll to the suggestion menu, where
it previously wouldn't work at all or only scroll when it wasn't in the
viewport.
- Ref: github/text-expander-element#50
- Ref: github/combobox-nav#75
- Resolves https://codeberg.org/forgejo/forgejo/issues/1990

(cherry picked from commit 27145be211ff782afe0910adbe200f126961f150)
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.

6 participants