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 native scrollIntoView method so it correctly accounts for paddings and scroll-margins #351

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

StefKors
Copy link
Contributor

@StefKors StefKors commented Nov 1, 2023

The DIY scroll into view calculations don't correctly account for paddings and scroll-margins. By using the native scrollInfoView method elements won't be clipped when they are styled by the users.

Before After
scrollIntoView-Before scrollIntoView-After

@csculley
Copy link
Collaborator

csculley commented Nov 1, 2023

This looks like a great change! It looks like some of the tests/linters failed, but they might be fixed just by using npm run lint. In addition, could you also bump the minor version number for this change? Thank you!

@StefKors
Copy link
Contributor Author

StefKors commented Nov 2, 2023

@csculley linted and version bumped 👍

@csculley
Copy link
Collaborator

csculley commented Nov 2, 2023

On second thought, maybe this could be just a patch version, but I'm not sure it matters too much. I've requested for some reviews, but this looks good to me!

Copy link
Member

@MadSpindel MadSpindel left a comment

Choose a reason for hiding this comment

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

LGTM

@csculley csculley merged commit f2801f2 into quill-mention:master Nov 2, 2023
1 check passed
Copy link
Collaborator

@thexeos thexeos left a comment

Choose a reason for hiding this comment

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

LGTM

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

4 participants