Skip to content

Fix editor toolbar#10673

Merged
ahukkanen merged 12 commits intodevelopfrom
fix/editor-template
Apr 19, 2023
Merged

Fix editor toolbar#10673
ahukkanen merged 12 commits intodevelopfrom
fix/editor-template

Conversation

@alecslupu
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu commented Apr 3, 2023 •
edited
Loading

🎩 What? Why?

When using the editor field type, we have the option of setting the toolbar to either "basic" or "full" depending on the needs.
Currently the Editor toolbar is displaying the video and image buttons no matter what, yet, based on the #10130 acceptance criteria, we need not to "have the video embed option in the WYSIWYG editor."

📌 Related Issues

Link your PR to an issue

Testing

📷 Screenshots

Please add screenshots of the changes you are proposing
Description

♥️ Thank you!

@alecslupu alecslupu force-pushed the fix/editor-template branch 2 times, most recently from 0783a8d to 5a49f8a Compare April 3, 2023 15:44
@alecslupu alecslupu closed this Apr 3, 2023
@alecslupu alecslupu reopened this Apr 5, 2023
@alecslupu alecslupu force-pushed the fix/editor-template branch from 5a49f8a to 1055d63 Compare April 5, 2023 16:08
@alecslupu alecslupu force-pushed the fix/editor-template branch from 1055d63 to 30814c3 Compare April 5, 2023 18:10
@alecslupu alecslupu requested a review from a team April 5, 2023 20:39
@alecslupu alecslupu marked this pull request as ready for review April 6, 2023 06:00
ahukkanen
ahukkanen requested changes Apr 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Nice work!

I only have some issues I'd like to propose regarding the addImage option which I think is useless. Also, we need to find out how to prevent the image dropping in Firefox when the images are not allowed.

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Apr 12, 2023 •
edited
Loading

The Firefox file dropping bug is because the image dropping is implemented in Firefox by default for contenteditable elements.

I made this fiddle to test it (open in Firefox and drop an image to the editable area):
https://jsfiddle.net/rveao430/

We can prevent this behavior in Quill by adding this line in the editor.js after the editor initialization:

quill.root.addEventListener("drop", (ev) => ev.preventDefault());

Note that this needs to be added ONLY when the image module is not enabled. When the image module is enabled, we should let the imageUpload module handle the image uploads normally.

@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Apr 12, 2023

The Firefox file dropping bug is because the image dropping is implemented in Firefox by default for contenteditable elements.

I made this fiddle to test it (open in Firefox and drop an image to the editable area): https://jsfiddle.net/rveao430/

We can prevent this behavior in Quill by adding this line in the editor.js after the editor initialization:

quill.root.addEventListener("drop", (ev) => ev.preventDefault());

Note that this needs to be added ONLY when the image module is not enabled. When the image module is enabled, we should let the imageUpload module handle the image uploads normally.

@ahukkanen in cbeff35 i have tried to consolidate the editor behaviour, by removing the form editor_images has value as you proposed, and also, tried to make addImage dependant of the type of the toolbar that is being used.

When the addImage is false, based on my tests, the pasting of the image is being disabled, and as well the drag and drop ( thanks for solution)!.

Also, i have tried to add some checks in the clipboard functionality to avoid exposing errors caused by the image uploader not being attached.

ahukkanen
ahukkanen requested changes Apr 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment •
edited by alecslupu
Loading

Choose a reason for hiding this comment

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

Nice work once again but after I reviewed #10467, I realized that in this PR (probably) we should also implement the toolbar changes as agreed at #10130. While reviewing that other PR, I also made notes where we should do changes:

  • Assemblies -> edit: for the closing_date_reason, use toolbar type content
  • Budgets -> edit budget: for the description, use toolbar type content
  • Conferences -> edit: for the registration_terms, use toolbar type content
  • Consultations -> edit question: for the title, use toolbar type basic (because this is rendered within an <h2>)
  • Forms (survey) -> edit: for the tos, use toolbar type content
  • Meetings registrations -> edit: for the registration_terms, use toolbar type content
  • Meetings, participant side -> new/edit: for the description, use toolbar type content
  • Meetings, participant side -> new/edit: for the registration_terms, use toolbar type content
  • Proposals, participant side -> new/edit, use toolbar type content
  • Sortitions -> edit, for witnesses, use toolbar type content
  • Sortitions -> cancel ("destroy"), for cancel_reason, use toolbar type content

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Apr 12, 2023

And one more oddity I found from initiatives that we discussed last week.

Currently e.g. at try.decidim.org, the initiative form looks like this (see the video):
Initiative edit

But even if I add a video there, it won't display on the initiative page, so I think this control is unnecessary.

After this PR, the image control would be also added to the toolbar which is not desired because the participant users are not allowed to upload images through the editor as this permission is only set for admins:

decidim/decidim-admin/app/permissions/decidim/admin/permissions.rb

Line 47 in 82b1336

allow! if permission_action.subject == :editor_image

Therefore, even if the image embedding would be possible for the participants, it won't work unless we change the permission logic (which is even more work).

Conclusion

As conclusion, let's use the toolbar type content also for initiative edit view. We should not do any harm doing this because the video embedding doesn't work in the current version either, although we show it in the toolbar.

@ahukkanen ahukkanen mentioned this pull request Apr 12, 2023
Merged
@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Apr 12, 2023

@ahukkanen, the work in this comments is supposed to be handled in a separate PR:

Considering that we already know this work is coming, i would leave it in the scope of that PR. ( I need this PR and #10467 in order to successfully perform the task)

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Apr 13, 2023

@alecslupu Are you sure about that? You will create a dependency on this PR also for the backport, so the backport will also eventually depend on this.

I'd handle that already in this PR if possible. It's only 12 views to change the toolbar type at (if I've catched them all in the list above + initiatives comment).

@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Apr 13, 2023

@alecslupu Are you sure about that? You will create a dependency on this PR also for the backport, so the backport will also eventually depend on this.

I could do it here, but i would really like to be able to go through each one of the view layers where the content is being rendered, then adjust the use of toolbar for each one of the fields...

ahukkanen
ahukkanen requested changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Also noticed an unnecessary line break while looking at the code.

@alecslupu alecslupu force-pushed the fix/editor-template branch from 2998f20 to 5e853ea Compare April 13, 2023 20:24
@alecslupu alecslupu force-pushed the fix/editor-template branch from 5e853ea to 7468acd Compare April 13, 2023 20:25
@alecslupu alecslupu requested a review from ahukkanen April 14, 2023 11:59
ahukkanen
ahukkanen requested changes Apr 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This works really nicely now. One thing I've noticed at the admin side:

  • Sortition "additional information" has content type toolbar when creating a new sortition but when editing it, it is full. In both cases it should be full. Note that the witnesses should remain content, as it already is.

Also on the participant side, I'm not quite sure what to do with these but the inconsistency feels really weird because some of these records can be modified from the admin panel too where we have different capabilities. Not so much about the multimedia controls (which is related to #10700) but more about the possibility to add subtitles (content toolbar type) as a participant which I would see useful in many places:

  • Debates new/edit: debate description
  • Initiatives new/edit: initiative description
  • Meetings new/edit
    • Meeting description
    • Meeting registration terms
  • Proposals new/complete/edit: proposal body

I think this would make a lot of sense eventually but this also comes with a problem when using the legacy design which has weird heading orders in some places. Maybe this is something we could incorporate into the next 0.28 release, where we (hopefully) won't have the problem of inconsistent headings...

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Apr 15, 2023

Regarding this comment I left above about the subtitles:

Also on the participant side, I'm not quite sure what to do with these but the inconsistency feels really weird because some of these records can be modified from the admin panel too where we have different capabilities. Not so much about the multimedia controls (which is related to #10700) but more about the possibility to add subtitles (content toolbar type) as a participant which I would see useful in many places:

  • Debates new/edit: debate description

  • Initiatives new/edit: initiative description

  • Meetings new/edit

    • Meeting description
    • Meeting registration terms

  • Proposals new/complete/edit: proposal body

I think this would make a lot of sense eventually but this also comes with a problem when using the legacy design which has weird heading orders in some places. Maybe this is something we could incorporate into the next 0.28 release, where we (hopefully) won't have the problem of inconsistent headings...

I posted a new feature request about that to Metadecidim:
https://meta.decidim.org/processes/roadmap/f/122/proposals/17253

I think we can potentially retake this when we have redesign ready.

ahukkanen
ahukkanen requested changes Apr 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

The sortitions spec is failing after the latest changes. I also had one doubt regarding the multimedia toolbar type that we are not using right now anywhere.

@ahukkanen ahukkanen merged commit f32861b into develop Apr 19, 2023
@ahukkanen ahukkanen deleted the fix/editor-template branch April 19, 2023 09:10
This was referenced Apr 19, 2023
Merged
Merged
@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Apr 20, 2023

FYI, I've added the missing labels (they're used for the changelog generator script)

@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Apr 20, 2023

FYI, I've added the missing labels (they're used for the changelog generator script)

I have added to backports as well.

@ahukkanen ahukkanen mentioned this pull request May 4, 2023
Open
@alecslupu alecslupu mentioned this pull request Jul 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen approved these changes

Assignees

No one assigned

Labels

module: admin module: assemblies module: blogs module: budgets module: conferences module: consultations module: core module: elections module: forms module: initiatives module: meetings module: pages module: proposals module: sortitions type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix WYSIWYG toolbar to enable/disable video editor

3 participants

@alecslupu @ahukkanen @andreslucena