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(placeholders): fix placeholders not being selectable #1045

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

abeforgit
Copy link
Member

@abeforgit abeforgit commented Nov 23, 2023


Overview

After the improvements to the gapcursor/position resolution from mouseclicks, placeholder nodes
threw erros when clicked on. This was because the position resolution logic tries to
find a position with a parent that is the same parent that the text cursor would have if
it would be standing where you clicked. It does this by walking upwards and comparing the value
of parent with the value of the node at the "inside" position (which prosemirror already calculates).
However, in the case of atom nodes, the first position checked already starts with a parent that
is the parent of the atom, meaning the walk upwards will never result in the correct position.
Additionally, the loop was missing a bounds check for depth=0, which caused the error

connected issues and PRs:

Setup

best way to test is npm link in the plugin repo, and try out the article structures and their
placeholders

How to test/reproduce

Challenges/uncertainties

Switching on the node being an atom is an educated and afaict a relatively conservative bet.
There might be more scenarios where the walk upwards will not find the right position.
Additionally this may result in some more scenarios where a good gapcursor can not be found,
since I don't have a reproduction of the cases the original code was trying to solve.

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@abeforgit abeforgit requested a review from elpoelma November 23, 2023 16:00
Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Looks good! I would just move the depth check to the while loop condition, as calling result.before also fails at depth 0.

@abeforgit abeforgit enabled auto-merge November 23, 2023 20:05
@abeforgit abeforgit merged commit 4226f8d into master Nov 23, 2023
@abeforgit abeforgit deleted the fix/placeholders-selectable branch November 23, 2023 20:07
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.

2 participants