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-4264: enable template comments in GN #552

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Conversation

abeforgit
Copy link
Member

@abeforgit abeforgit commented Aug 8, 2023

Overview

bumps plugins to 10.0.0
updates config to reflect the interface changes
removes outdated references to templateVariablePlugin from environment.js
enables template-comments-plugin on both editor configs

connected issues and PRs:

GN-4264

Setup

just run it

How to test/reproduce

check template plugin, and variable plugins

Challenges/uncertainties

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 review from elpoelma and x-m-el August 8, 2023 15:02
@abeforgit abeforgit added the enhancement New feature or request label Aug 8, 2023
@x-m-el
Copy link
Contributor

x-m-el commented Aug 9, 2023

Probably want to "wait" on PR to hide template comments when publishing before merging this? So we don't get template comments in published data.

Both PR for hiding template comments in frontend GN and here template comments are added to editor, so will need to look out for at merging.

Is there any clear indication if we want template comments to be insertable in GN, like in agendapoints? Because I'd think there isn't more example/info to be given at this stage. Or are they meant as any general "extra information" (so an actual comment :p ), to give some information about what to do in a certain agenda point?

Note that at this point they are hidden when previewing an agenda and are only shown during editing. So as far as I can see, the use would only be if an agenda point is created/edited in multiple stages and not at once.

Copy link
Contributor

@x-m-el x-m-el left a comment

Choose a reason for hiding this comment

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

still have to import @import "template-comments-plugin"; in app styles (app.scss). Note: was missing from documentation to add this, this is added in a PR for template comment readme update.

For the rest is good. Tested comments and variables in agenda and regulatory statement 👍

@abeforgit
Copy link
Member Author

Yeah I considered waiting, but given that the corresponding feature is already active on GN QA, if templates containing them will be used now in GN they will simply render as flat text, which I though was about as bad as them showing up in publications, but this at least can get the testers going

I was also debating whether or not GN users had to be able to insert them or not, figured why not. From what I gather the "edit in multiple stages" workflow is a real usecase, and a finished regulation/agenda item can be completed by different people.

And on principle I want to move away from these "if I delete this, I can't put it back" scenarios where its there in the template, but the GN user lacks the tools to make that "thing" they see. They can already edit the comments anyway, which I'm completely fine with as well.

Hidden on preview is also perfect imo, the reader never needs to see them, only the person editing the document should.

And thanks for the hint about the css, I almost made a ticket for cleaning up the styling 🙈

@abeforgit
Copy link
Member Author

so will need to look out for at merging

merge conflicts are like spiders: they're more afraid of you than you are of them

@abeforgit abeforgit merged commit e5d9600 into master Aug 9, 2023
@abeforgit abeforgit deleted the chore/bump-plugins branch August 9, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants