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

Replace snippet placeholder when deleting last snippet #495

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

piemonkey
Copy link
Collaborator

Overview

Tried to reduce some of the differences in naming and arguments between snippet nodes and snippet placeholders while doing this, so it's easier to do other work that pulls them together in the future but without re-writing everything. Also fixes commas in snippet list names.

Should be backwards compatible but does (can) not fix some historical issues that will remain in old documents. Namely:

  • Snippets inserted in old documents don't know which placeholder they 'came from', so will be handled as separate 'groups'. For example, if you created a document from a template including a placeholder, inserted two snippets, then saved; the new code will now, when deleting either of those snippets, replace it with a placeholder. This can easily be deleted, by pressing backspace, so this shouldn't be too much of a problem.
  • Snippet placeholders which include a snippet with a , in the name will remain broken. There's no reasonable way for us to figure out that this has happened, so we just handle it as before.
connected issues and PRs:

Jira ticket: https://binnenland.atlassian.net/browse/GN-5003

Setup

N/A

How to test/reproduce

Create a document with placeholders, multi-snippet placeholders and placeholders with commas in the list names and play around inserting and deleting snippets and refreshing.

Challenges/uncertainties

N/A

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

@piemonkey piemonkey requested review from lagartoverde, elpoelma and abeforgit and removed request for lagartoverde and elpoelma November 5, 2024 14:12
@abeforgit
Copy link
Member

completely fair compromises wrt backwards compat, will have a look tomorrow

@piemonkey piemonkey mentioned this pull request Nov 5, 2024
7 tasks
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