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

Add rich text format for custom fields #18481

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cconard96
Copy link
Contributor

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

Add option for custom text fields to support rich-text and images.

@cconard96 cconard96 force-pushed the customasset/richtext_field branch from d8ce7b9 to 40c693c Compare December 5, 2024 22:57
@cconard96 cconard96 marked this pull request as ready for review December 7, 2024 10:33
@cedric-anne cedric-anne requested a review from orthagh December 9, 2024 15:33
@orthagh
Copy link
Contributor

orthagh commented Dec 10, 2024

Maybe not related to this pr, but I can't test it without this issue fixed:
image

@cedric-anne
Copy link
Member

@cconard96 Please rebase to solve conflicts.

@cconard96 cconard96 force-pushed the customasset/richtext_field branch from 7cfbeeb to 272bbf9 Compare January 8, 2025 15:40
@cconard96 cconard96 requested a review from cedric-anne January 8, 2025 15:40
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

  1. The rendering of the default value is not correct:
    image

  2. When reopening the field configuration form, the default value is not rendered correctly in the modal :
    image

  3. Images are not converted correctly into document. This is usally done with a call to the CommonDBTM::addFile() in the post* methods.

  4. We disabled the ability to put images in the task/followups/solutions/... templates because it was complicated to handle the copy of the corresponding document when an item was created using these templates. I think we should avoid usage of images in the default value here for the same reasons.

@cedric-anne cedric-anne self-assigned this Jan 27, 2025
@orthagh
Copy link
Contributor

orthagh commented Jan 27, 2025

Pending topic: if someone changes a field from rich-text to text-only, what do we do with the content?

  • nothing, we consider the case rare
  • we make a transformation to remove all tags and keep only text (Glpi\RichText\RichText::getTextFromHtml)
  • we remove the slider and move to 2 field types to avoid this case (it may ease migration also as plugins separate these types of fields also)

@cconard96 your opinion?

@cedric-anne cedric-anne force-pushed the customasset/richtext_field branch from 79f5158 to 52529c8 Compare January 27, 2025 15:25
@cconard96
Copy link
Contributor Author

Pending topic: if someone changes a field from rich-text to text-only, what do we do with the content?

* nothing, we consider the case rare

* we make a transformation to remove all tags and keep only text (`Glpi\RichText\RichText::getTextFromHtml`)

* we remove the slider and move to 2 field types to avoid this case (it may ease migration also as plugins separate these types of fields also)

@cconard96 your opinion?

My opinion is not to bother modifying any of the values in the database if the field is switched between plain/rich text. If the field is plain text, we should force the displayed value to go through "getTextFromHtml". This way, simply switching to plain text and then back to rich text won't ruin existing values but if an asset is saved the plain version will be saved.

Second option would be just blocking changing this option once a field is created but it could limit usability. Admins would need to create a new field if they change their mind later and we don't offer any way to migrate data from one field to another.

@cedric-anne
Copy link
Member

My opinion is not to bother modifying any of the values in the database if the field is switched between plain/rich text. If the field is plain text, we should force the displayed value to go through "getTextFromHtml". This way, simply switching to plain text and then back to rich text won't ruin existing values but if an asset is saved the plain version will be saved.

Seems a good way to handle it.

@orthagh
Copy link
Contributor

orthagh commented Jan 29, 2025

let's go with that

@cconard96 cconard96 force-pushed the customasset/richtext_field branch from 52529c8 to f35ba39 Compare January 30, 2025 15:45
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