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

feat(forms): adding comment block #17153

Merged
merged 7 commits into from
May 23, 2024

Conversation

ccailly
Copy link
Member

@ccailly ccailly commented May 21, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets

Adds a new comment block feature to the form editor.
The comment block allows users to add comments to a form section.

In the form editor :
image

In the form editor (focused) :
image

In the form preview :
image

@ccailly ccailly self-assigned this May 21, 2024
@ccailly ccailly added this to the 11.0.0 milestone May 21, 2024
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

I am unsure about the "Block" name to represent Question + Comment.

I think it is a bit ambiguous as the form main details and the sections also looks like "blocks" in the interface and thus someone might think they are a "Block" too.

With that being said, I do not have any better suggestion.
If someone else has one, please let us know :)

templates/pages/admin/form/form_comment.html.twig Outdated Show resolved Hide resolved
css/includes/components/form/_form-editor.scss Outdated Show resolved Hide resolved
css/includes/components/form/_form-editor.scss Outdated Show resolved Hide resolved
css/includes/components/form/_form-editor.scss Outdated Show resolved Hide resolved
js/form_editor_controller.js Show resolved Hide resolved
js/form_editor_controller.js Outdated Show resolved Hide resolved
src/Form/Block.php Outdated Show resolved Hide resolved
src/Form/Comment.php Outdated Show resolved Hide resolved
src/Form/Question.php Outdated Show resolved Hide resolved
templates/pages/admin/form/form_comment.html.twig Outdated Show resolved Hide resolved
ccailly and others added 2 commits May 22, 2024 11:59
Co-authored-by: Adrien Clairembault <42734840+AdrienClairembault@users.noreply.github.com>
Comment on lines +633 to +638
if ($comment_data["_use_uuid_for_sections_id"]) {
// This question was added to a newly created section
// We need to find the correct section id using the temporary UUID
$uuid = $comment_data['forms_sections_id'];
$comment_data['forms_sections_id'] = $_SESSION['form_editor_sections_uuid'][$uuid] ?? 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is the kind of logic that will be hard to maintain. It requires additionnal inputs on the form side and a temporary storage in session data.

Maybe it could be handled in a simplier way by nesting data in the POST payload, for instance

_sections[0]=[...]
_comments[0]=[...]

may be changed to

_sections[0]=[
    ...
    _comments[0]=[...]
]

The comment would be part of the section payload and it would be therefore easier to attach it to the section. Also, the payload would probably be easier to read.

As this logic is already used by questions, it is not a blocker for the current PR, but I think it could be interesting to simplify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was the original behavior in the first forms iteration (nested arrays).
There was some issues with it but I don't remember exactly what.

I agree the uuid stuff is not ideal too, a better solution would be welcome.

@cedric-anne cedric-anne merged commit 5b89749 into glpi-project:main May 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants