-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Update editable.tsx: auto-focus fix for end of line with several chil… #4695
base: main
Are you sure you want to change the base?
Conversation
…dren for input div Update editable.tsx: auto-focus fix for end of line with several children for input div
|
if (ref.current && autoFocus) { | ||
if(window.getSelection && document.createRange) { | ||
const range = document.createRange(); | ||
node.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is node
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akonsu Sorry, node was a reference to ref.current in my other sandbox. Will correct it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. Please add a changeset when you have a moment and then we can land this.
if (ref.current && autoFocus) { | ||
if(window.getSelection && document.createRange) { | ||
const range = document.createRange(); | ||
ref.current.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it essential to call ref.current.focus()
here and not at the end of this if
clause? Maybe we can move this call out of the if
statement (from both this clause and the else
clause)?
} | ||
}, [autoFocus]) | ||
if (ref.current && autoFocus) { | ||
if(window.getSelection && document.createRange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prettify the diff. This if
needs a space after it.
@GrinchakAndrew any chance you have time to wrap this one up. It looks like it needs a linting fix and there are a couple of comments to address. Thanks! |
Hello, i am |
Unfortunately because you created your PR from your main branch it's not easy to clone your PR locally. But I'll make it work. |
* Run prettier * Null checks for selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is causing a regression in the markdown-shortcuts example and is thus failing our integrations tests. Will look when I get a chance, unless someone else has time to track this down before I do.
Andrew lives in Ukraine so I know he won't be able to finish this anytime soon. We discovered this issue on a project together so I'll get this to the finish line. Looks like |
Hi @JustinBeaudry, thanks, I hope @GrinchakAndrew is ok. With respect to running locally, they run for me with 14.15.5 on macOS, but there's nothing special about that version. If they won't run locally you can commit to the PR which will trigger a re-run which is not ideal but better than nothing. Feel free to ping me on Slack ( https://join.slack.com/t/slate-js/shared_invite/zt-f8t986ip-7dA1DyiqPpzootz1snKXkw ) if you're stuck. |
My attempt at using GitHub's tooling to resolve merge conflicts failed badly here. I will look at this later. |
Solution resolves cursor position in the Editor with autofocus set to true, when cursor pos is 0 instead of end of line.