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.

Sign up for GitHub

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

Jump to bottom

fix: autoscroll when block is bigger than viewport #3746

Merged
merged 5 commits into from
Mar 31, 2021
Merged

fix: autoscroll when block is bigger than viewport #3746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af9faf0
fix: autoscroll when block is bigger than viewport
Jun 20, 2020
6178862
Merge branch 'master' into scroll-when-block-is-bigger-than-viewport
ianstormtaylor Mar 31, 2021
bbad026
Create lazy-trains-sell.md
ianstormtaylor Mar 31, 2021
db3b47b
Update lazy-trains-sell.md
ianstormtaylor Mar 31, 2021
95a235d
Update lazy-trains-sell.md
ianstormtaylor Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lazy-trains-sell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'slate-react': patch
---

Fix editor auto-scrolling behavior when a block is bigger than the viewport.
4 changes: 4 additions & 0 deletions packages/slate-react/src/components/editable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,14 @@ export const Editable = (props: EditableProps) => {
)
}
const leafEl = newDomRange.startContainer.parentElement!
leafEl.getBoundingClientRect = newDomRange.getBoundingClientRect.bind(
newDomRange
)
Copy link

Choose a reason for hiding this comment

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

when testing this fix, did you scroll around a lot? Worried about performance issues when building and destroying this every time you scroll

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 haven't found any performance issues so far. If that was the case however I think we would see the issue on typing rather than on scrolling given this piece of code is only called when the selection changes, not the scroll. I believe this should be a cheap operation either way. Or at least probably cheaper than the DOM access operations that happen on toDOMRange sequential to this.

scrollIntoView(leafEl, {
scrollMode: 'if-needed',
boundary: el,
})
delete leafEl.getBoundingClientRect
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 this throws a typescript error could you elaborate why this is needed @gztomas?

Copy link
Contributor Author

@gztomas gztomas Apr 22, 2021 •
edited

Choose a reason for hiding this comment

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

@juliankrispel this restores the override happening at line 208, so any future call to leafEl.getBoundingClientRect will get the native getBoundingClientRect implementation from leafEl prototype chain. This is for cleanup purposes, as this code doesn't know who else might need to call getBoundingClientRect for this element. I guess TS can't know that even deleting this, getBoundingClientRect will still be available, from the prototype.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for explaining @gztomas

} else {
domSelection.removeAllRanges()
}
Expand Down