fix: Scrolling behavior when remote clients are editing #33
Conversation
| @@ -252,7 +255,7 @@ export class ProsemirrorBinding { | |||
| unrenderSnapshot () { | |||
| this.mapping = new Map() | |||
| this.mux(() => { | |||
| const fragmentContent = this.type.toArray().map(t => createNodeFromYElement(/** @type {Y.XmlElement} */ (t), this.prosemirrorView.state.schema, this.mapping)).filter(n => n !== null) | |||
| const fragmentContent = this.type.toArray().map(t => createNodeFromYElement(/** @type {Y.XmlElement} */(t), this.prosemirrorView.state.schema, this.mapping)).filter(n => n !== null) | |||
There was a problem hiding this comment.
All of the edits from here down are a result of running standard --fix before committing vvv
There was a problem hiding this comment.
I care about these things more than I should. I use the space consistently between projects, so I need to reverse your changes every time I merge your PRs.. (small price because your PRs are much appreciated)
The package wouldn't publish if that was not validly formatted. I use standard v16 (the current version). This package uses v12, which also doesn't define that rule.
For the future, you could just run npm run lint using the intended version.
There was a problem hiding this comment.
I need to reverse your changes every time I merge your PRs..
I'm not trying to change anything, I'm just running the included linters. It would be very useful if you'd consider setting up CI on these projects, this is the only project I contribute to that doesn't enforce linting rules automatically, it would save your time in the long run I'm sure.
There was a problem hiding this comment.
You are not running standard@v12. And a CI can't fix that. In the future, instead of using your linter, I'm just asking you to run npx standard --fix using the shipped version of standard.
I would be very surprised if the linter works differently on your computer. Maybe it does. Maybe you setup some custom rules. Then please make my life easier by fixing the linting manually.
Your code is technically valid standard code. So a CI can't enforce you not to run your linter.
I'm all for setting up CI. For different reasons though.
There was a problem hiding this comment.
I don't want to seem like I'm arguing, I just want to be clear – I'm absolutely running standard@v12, it's installed in node_modules as per the package.json and running npx standard --fix from the command line results in this exact same output 🤷
There was a problem hiding this comment.
It seems to be related to your system. I'm running these packages on Linux, Mac, and Windows with virtually any Node version (10, 12, and 14). Here, I also added travis CI to this project:
The project is linted correctly, and I'd like not to change this space everywhere. In that case please don't run the --fix command. Maybe you can figure out the problem and report an issue to the standard project.
src/plugins/sync-plugin.js
Outdated
| @@ -228,14 +228,17 @@ export class ProsemirrorBinding { | |||
| } | |||
|
|
|||
| _isDomSelectionInView () { | |||
| // We only want to consider a dom selection that's within the editor itself | |||
| const documentElement = dom.doc.documentElement | |||
| if (!this.prosemirrorView.dom.contains(documentElement)) return false | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
That doesn't make sense to me.. How can the document be inside the proseMirrorView? This doesn't check selections at all.
So is this the only actual change? Is the rest just linting?
There was a problem hiding this comment.
It seems I misunderstood, will take another bash at it in the next few hours
|
Somewhat debugging in public here, it seems like when the selection is collapsed and the caret is at the beginning of the line getClientBoundingRect returns zero for all values, if you have an expanded selection it's correct and the bug does not reproduce.
|
|
That seems very odd.. |
| // return zero values if the selection is collapsed at the start of a newline | ||
| // see reference here: https://stackoverflow.com/a/59780954 | ||
| const rects = range.getClientRects() | ||
| if (!rects.length) { |
There was a problem hiding this comment.
Because this logic is only executed when the selection range is "empty" it seems like a safe addition considering the editor being in focus is guarded earlier. I'm not seeing any visual artifacts testing across browsers.
|
Thanks for this PR! I copied the relevant changes and published a new release. The issue should be fixed in y-prosemirror@1.0.5 |
Only consider the dom selection if it's within the editor bounds.
closes #31