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

Snippet placeholder redesign #496

Merged
merged 9 commits into from
Nov 8, 2024
Merged

Conversation

piemonkey
Copy link
Collaborator

Overview

Continued a little of the refactoring from replacing the snippet placeholder, so that the snippet placeholder component is now used inside a snippet component. This means that I could re-use the insert logic from the snippet buttons and if we wanted in the future we could include buttons in the same style as for inserted snippets. Since I was not including buttons for now, I was able to use @elpoelma 's redesign implementation work almost untouched.

For consistency, I move the snippet list ids information to RDFa for inserted snippet nodes.

connected issues and PRs:

Builds on #495
Jira ticket: https://binnenland.atlassian.net/browse/GN-5117

Setup

N/A

How to test/reproduce

Insert, replace, delete snippets and snippet placeholders with different configurations and make sure everything works as expected. The expected interactivity is different depending on which sample page:

  • Besluit: It's only possible to insert snippets using the in-node insert button. placeholders cannot be inserted
  • Reg statement: The in-node insert snippet button is hidden, but the sidebar button remains.
  • Snippet edit: The in-node snippet insert button is shown, the sidebar button is not there. Placeholders can be inserted.

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

Snippet placeholder node now uses the snippet nodeview component, which
in turn shows the snippet placeholder component if relevant. This allows
for code re-use between the two for inserting snippets
@piemonkey piemonkey force-pushed the feat/snippet-placeholder-redesign branch from 3f79e36 to 2e3c09f Compare November 5, 2024 17:50
Copy link
Contributor

@lagartoverde lagartoverde left a comment

Choose a reason for hiding this comment

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

Left some minor comments

app/styles/snippet-plugin.scss Outdated Show resolved Hide resolved
app/styles/snippet-plugin.scss Outdated Show resolved Hide resolved
app/styles/snippet-plugin.scss Outdated Show resolved Hide resolved
tests/dummy/app/controllers/snippet-edit-sample.ts Outdated Show resolved Hide resolved
@piemonkey
Copy link
Collaborator Author

@elpoelma are you looking at these comments or should I?

Base automatically changed from feat/snippet-keep-placeholder to master November 7, 2024 12:34
@elpoelma
Copy link
Collaborator

elpoelma commented Nov 7, 2024

@elpoelma are you looking at these comments or should I?

You can do it if you want :) I am currently working on an editor PR which fixes the specificity css styling regression which should remove the need of the !important modifier.

Copy link
Contributor

@lagartoverde lagartoverde 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 to me

@elpoelma elpoelma merged commit c291f87 into master Nov 8, 2024
2 of 3 checks passed
@elpoelma elpoelma deleted the feat/snippet-placeholder-redesign branch November 8, 2024 09:47
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.

3 participants