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: default scroll selection #4652

Merged
merged 5 commits into from
Nov 16, 2021
Merged

Conversation

karthikcodes6
Copy link
Contributor

@karthikcodes6 karthikcodes6 commented Nov 10, 2021

Description
Disabled the auto scroll behaviour when it's on selection

Issue
Fixes: (link to issue)
#4647

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2021

🦋 Changeset detected

Latest commit: 7efec9b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@karthikcodes6
Copy link
Contributor Author

karthikcodes6 commented Nov 10, 2021

Let me know if there is any change required. I know we can pass custom function to override this. But it's better to provide the default behaviour correctly within slate itself.

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Hit approve prematurely... in general I think you're on the right track, but there's likely a better way to do this. Just landed after an overnight flight so I'll respond with feedback once my brain is working fully. :)

@@ -1326,6 +1326,9 @@ const defaultScrollSelectionIntoView = (
editor: ReactEditor,
domRange: DOMRange
) => {
if (editor.selection?.anchor?.path[0] !== editor.selection?.focus?.path[0]) {
Copy link
Contributor

@nemanja-tosic nemanja-tosic Nov 10, 2021

Choose a reason for hiding this comment

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

Not sure if the check is for an expanded range, if so Range.isExpanded should be used. If not, a variable should be extracted so that the conditional is a bit more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot that we have helper methods.

@karthikcodes6
Copy link
Contributor Author

@dylans any update on this PR?

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Overall I think this change is fine (see my grammar suggestion), but I'm not sure if it fully fixes the issue as there might be other scenarios leading to this issue. But let's go ahead and land this and see if there are remaining scenarios that break scrolling.

scrollMode: 'if-needed',
})
delete leafEl.getBoundingClientRect
// This was affecting the selecting multi blocks and dragging behaviour so, enabled only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// This was affecting the selecting multi blocks and dragging behaviour so, enabled only
// This was affecting the selection of multiple blocks and dragging behavior, so enabled only

@dylans
Copy link
Collaborator

dylans commented Nov 15, 2021

P.S. This one needs a changeset as well, thanks!

@karthikcodes6
Copy link
Contributor Author

I added changeset, thanks again.

@dylans dylans merged commit 95389ed into ianstormtaylor:main Nov 16, 2021
@github-actions github-actions bot mentioned this pull request Nov 16, 2021
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.

3 participants