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-4441: Hide template comments via CSS in all previews #545

Conversation

x-m-el
Copy link
Contributor

@x-m-el x-m-el commented Aug 2, 2023

Overview

Hide template comments when previewing documents (but keep showing when editing/filling them in).
Template comments are only needed to be shown when editing a document.

connected issues and PRs:

This PR needs #541 which updates the editor packages. Note that this PR is a PR to master, split from #541, which means it also includes the #541 changes. The changes for hiding template comments can all be found in app/styles/project/_c-editor-preview.scss
GN-4441
Changes in the publishing service: lblod/notulen-prepublish-service#105

Setup

  1. checkout GN-4441 Remove template comments from anything requested notulen-prepublish-service#105 and add it as your preimporter in e.g. docker-compose.override.yml of app-gelinkt-notuleren. See https://github.com/lblod/notulen-prepublish-service#development-and-testing
  2. run frontend (ember serve --proxy ...)

How to test/reproduce

Make sure all template comments are hidden (except for where editing the document).
To easily add them do a document, add

<TemplateCommentsPlugin::Insert @controller={{this.controller}}/>

to an editor.

Challenges/uncertainties

Hiding is done via css, which is the most easy part (and quickest, as there is no document changing).
It was added to all "preview" classes, because it is unclear where they might be used and end up in. So safest is the always hide them.

Ruben added 2 commits August 2, 2023 12:20
…wing-signing-and-publishing-in-the-publishing-service

# Conflicts:
#	package-lock.json
#	package.json
@elpoelma elpoelma changed the base branch from master to update-editor-packages August 2, 2023 11:33
@elpoelma elpoelma changed the base branch from update-editor-packages to master August 2, 2023 11:33
@elpoelma elpoelma changed the base branch from master to update-editor-packages August 3, 2023 12:10
…mments-on-previewing-signing-and-publishing-in-the-publishing-service
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.

The template comments seem to be correctly hidden on the meeting page. On the publishing page, I get several JSON.parse errors, and the decisions/agendapoints do not load.

@elpoelma
Copy link
Contributor

elpoelma commented Aug 3, 2023

The template comments seem to be correctly hidden on the meeting page. On the publishing page, I get several JSON.parse errors, and the decisions/agendapoints do not load.

Update: possibly an issue with my local setup, will check.

Base automatically changed from update-editor-packages to master August 3, 2023 12:45
@elpoelma
Copy link
Contributor

elpoelma commented Aug 3, 2023

The template comments seem to be correctly hidden on the meeting page. On the publishing page, I get several JSON.parse errors, and the decisions/agendapoints do not load.

Update: possibly an issue with my local setup, will check.

Sadly not a local issue, seems to be an issue with the response that is generated from the prepublish-service

@x-m-el
Copy link
Contributor Author

x-m-el commented Aug 7, 2023

@elpoelma I tried to test it myself:
I can't seem to reproduce any Json.parse errors. Maybe because my document is too simple?
On publishing page (pressing publishing button on Notulen page?) it gets shown as expected: with template comment on master, without them on my branch, without any errors and the agenda is shown.

Could you try and create a reproducable example that gives an error/has a problem that I can test out? I might miss things of how gelinkt-notuleren should work, so that's why I look at differences between master and this branch instead of what I think it should do :)

@elpoelma
Copy link
Contributor

@x-m-el I created a video in which I could reproduce the bugs I encountered.
Screencast from 2023-08-10 13-49-39.webm

@elpoelma elpoelma self-requested a review August 17, 2023 08:47
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.

Update: it seems the issue was on my side. As I did not set NODE_ENV to development when overriding the preimporter-service, sorry. I tested it with a meeting containing an agendapoint which contains a regulatory statement with template comments. The template comments do not show up on the meeting page or on the publishing page.

…wing-signing-and-publishing-in-the-publishing-service

# Conflicts:
#	app/controllers/agendapoints/edit.js
#	app/controllers/regulatory-statements/edit.js
@x-m-el x-m-el merged commit 8fa5f7a into master Aug 17, 2023
@x-m-el x-m-el deleted the GN-4441-remove-template-comments-on-previewing-signing-and-publishing-in-the-publishing-service branch August 17, 2023 14:17
@x-m-el x-m-el added the enhancement New feature or request label Aug 23, 2023
@x-m-el x-m-el changed the title Hide template comments in all previews GN-4441: Hide template comments via CSS in all previews Aug 23, 2023
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