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: regression caused by triple click fix #4498

Merged

Conversation

bytrangle
Copy link
Contributor

@bytrangle bytrangle commented Sep 8, 2021

fixes #4490

Description
Introducing fix for triple-click selection in this PR caused some regressions that I didn't expect.

This new PR will alleviate that.

Issue
Some of the issue caused by the triple click fix include, but not limited to:

Context
There are two problems with my triple-click fix:

  • The condition for checking if an action is a triple-click is too broad.
  • The logic for resetting focusNode of a selection is not fail-proof. The building block of the content component in a Slate editor are HTML element with attribute data-slate-node="element". They may contain child elements data-slate-string, but not always. The current logic assumes that Element always contains String elements, which leads to the crash when selecting void nodes.

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.)

Only reset the focus node of a selection when the node with attribute `data-slate-string` is defined
@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2021

🦋 Changeset detected

Latest commit: 8ebf4c7

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

@bytrangle
Copy link
Contributor Author

bytrangle commented Sep 8, 2021

Thank you @dylans for the edit. Are the failed tests acceptable? They passed on my local machine before the addition of changeset.

@dylans
Copy link
Collaborator

dylans commented Sep 8, 2021

Thank you @dylans for the edit. Are the failed tests acceptable? They passed on my local machine before the addition of changeset.

They appear to be timeouts in the CI/CD testing environment, so I’m not going to block based on that.

@dylans dylans merged commit f5c0cbd into ianstormtaylor:main Sep 8, 2021
@github-actions github-actions bot mentioned this pull request Sep 8, 2021
@TheSpyder
Copy link
Collaborator

Thanks for doing that quickly after it was discovered! I have a small request.

This is a fix for a change that wasn't released, so it doesn't need a changeset. The original fix did need a changeset but one wasn't added. As such, your one entry in the changelog now looks a bit weird.

Would you mind creating a new PR that removes this changeset and adds one for the #4455 fix? I would do it myself but then you'd lose credit.

@bytrangle bytrangle mentioned this pull request Sep 9, 2021
5 tasks
dylans added a commit to dylans/slate that referenced this pull request Sep 13, 2021
* fix: check if data-slate-tring node is not null

Only reset the focus node of a selection when the node with attribute `data-slate-string` is defined

* fix: rewrite logic for checking triple click

* Add changeset

Co-authored-by: Dylan Schiemann <dylan@dojotoolkit.org>
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.

Crash during mouse selection on Chrome over multiple nodes
3 participants