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

GN-4732: ensure variables are selected/focused after insertion #392

Conversation

elpoelma
Copy link
Collaborator

@elpoelma elpoelma commented Feb 27, 2024

Overview

This PR introduces some changes into what happens after inserting a variable node.

Before this PR:

  • A text-cursor is set just after the inserted variable (the variable is not focused)
  • This causes different issues on both chromium and firefox
    • On chromium: no text-cursor is shown
    • On firefox: a text-cursor is shown, but does not act as a real selection (typing does not work)

Modifications introduced in this PR:

  • After variable insertion, the variables is selected/focused, allowing users to directly interact with them
  • Similar to how addresses/links behave
  • Works on both firefox and chromium
connected issues and PRs:

Solves GN-4732

How to test/reproduce

  • pnpm start
  • Insert a variable (text, number, codelist, location or date)
  • Notice that the variable is selected/focused after insertion, you can directly interact with them

Challenges/uncertainties

I can also add this fix to the stable version of the plugins package, to ensure we can release this fix when releasing to prod.

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check if dummy app is correctly updated
  • Check cancel/go-back flows
  • changelog
  • npm lint
  • no new deprecations

@elpoelma elpoelma added the bug Something isn't working label Feb 27, 2024
Copy link
Contributor

@dkozickis dkozickis left a comment

Choose a reason for hiding this comment

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

One non-blocking comment. Otherwise works 🙏

addon/components/variable-plugin/codelist/insert.ts Outdated Show resolved Hide resolved
@elpoelma elpoelma force-pushed the GN-4732-cursor-location-after-inserting-variables branch from 91b7453 to d34a188 Compare February 29, 2024 13:18
@elpoelma elpoelma force-pushed the GN-4732-cursor-location-after-inserting-variables branch from d34a188 to 8f942f2 Compare February 29, 2024 13:22
Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

would be even better if the placeholder inside the variables that have one would be selected, you think that's doable?

@elpoelma
Copy link
Collaborator Author

elpoelma commented Mar 7, 2024

would be even better if the placeholder inside the variables that have one would be selected, you think that's doable?

That would be nice indeed:

  • At the moment, this is quite hard to do, as the selection/state of the embedded editor/main editor are quite seperated
  • I have some ideas to improve the integration of embedded editor, in order to ensure that selections/states are shared in a better way. In this case the selection of the main editor would also be updated when changing the selection in the embedded editor. The developer using our editor would not even need to care/be aware of the embedded editors. This approach would be similar to https://prosemirror.net/examples/codemirror/

@abeforgit
Copy link
Member

sounds good, lets merge for now then since it's already an improvement

@abeforgit abeforgit self-requested a review March 8, 2024 11:21
@elpoelma elpoelma merged commit 442efd8 into feat/rdfa-editing-ui Mar 8, 2024
1 of 2 checks passed
@elpoelma elpoelma deleted the GN-4732-cursor-location-after-inserting-variables branch March 8, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants