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.

Sign up for GitHub

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

Jump to bottom

Add toolbox on top of each richeditor widget #212

Merged
merged 11 commits into from
May 25, 2024
Merged

Add toolbox on top of each richeditor widget #212

merged 11 commits into from
May 25, 2024

Conversation

kgonella
Copy link
Contributor

@kgonella kgonella commented Feb 19, 2024 •
edited
Loading

This PR is a proposal to address the issue #93

First commit is only a POC for now to open a discussion on how to implement it.

What is missing :

  • Verify if all components pasted follow the tags rules (filter out the bad ones)
  • Alert the user that he will replace the actual content (maybe open a modal with choices : overwrite, add at the end, do nothing)

TODO

  • Correct issue on delete all
  • Is it possible to disable the button Paste all if nothing in the clipboard ?

@kgonella kgonella force-pushed the feature/copy-paste-all-contents branch from e122274 to b60c151 Compare February 19, 2024 14:27
@jacquesbh jacquesbh requested a review from delyriand February 21, 2024 19:22
@madamebiz madamebiz added the Status: needs review Status: needs review label Feb 21, 2024
@kgonella
Add confirmation modal to avoid bad actions likes
392081f 
- Trashes all elements
- Overwrite all elements
@kgonella kgonella marked this pull request as ready for review February 27, 2024 08:43
@maximehuran maximehuran requested a review from jacquesbh March 1, 2024 08:25
@maximehuran maximehuran force-pushed the feature/copy-paste-all-contents branch from 392081f to 2db72ca Compare March 11, 2024 08:17
@maximehuran maximehuran self-requested a review March 11, 2024 08:17
maximehuran
maximehuran requested changes Mar 11, 2024
View reviewed changes
Copy link
Member

@maximehuran maximehuran left a comment

Choose a reason for hiding this comment

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

I have an issue when I try to remove all elements.

Enregistrement.de.l.ecran.2024-03-11.a.09.16.12.mov

It's removed but after it appears again.

@madamebiz madamebiz added Status: needs update This Issue/PR needs update and removed Status: needs review Status: needs review labels Mar 11, 2024
@kgonella
Copy link
Contributor Author

Hello @maximehuran, thanks for the review.

Can you give me some context, I have tried to do the same test as you and I can't reproduce the bug.
I have tried under Firefox / Chrome on Ubuntu.

@kgonella
Copy link
Contributor Author

kgonella commented Apr 2, 2024

Hello @maximehuran, any news ?

@maximehuran
Copy link
Member

Hi @kgonella !

I am on Firefox on Mac.
I will try again to check if the issue still exist

@maximehuran maximehuran force-pushed the feature/copy-paste-all-contents branch from 2db72ca to ef2688e Compare April 12, 2024 15:54
@maximehuran
Copy link
Member

I reproduced the issue on Firefox and Chrome :

Enregistrement.de.l.ecran.2024-04-12.a.17.52.56.mov
  • Copy all elements from somewhere
  • Paste all on an editor which already contains 1 element inside
  • The pasted element will replace the existing one
  • Now click on delete all button, you will have the same behaviour as the video.

@kgonella
Copy link
Contributor Author

Ok I fixedd the issue. As the modal used was the same on multiple calls, the "click' events were stacked and called at the same time. As we can remove event listener easily in javascript, the best way I found was to clone the node and remove the old one.

@jacquesbh jacquesbh requested a review from maximehuran April 25, 2024 09:54
@madamebiz madamebiz added Status: needs review Status: needs review and removed Status: needs update This Issue/PR needs update labels Apr 25, 2024
@jacquesbh jacquesbh requested a review from delyriand April 25, 2024 09:55
@maximehuran maximehuran force-pushed the feature/copy-paste-all-contents branch from a98932a to a93b65e Compare May 9, 2024 13:23
@maximehuran maximehuran requested review from maximehuran and delyriand and removed request for delyriand, jacquesbh and maximehuran May 9, 2024 13:23
@maximehuran
Copy link
Member

Hi @kgonella 👋

I rebased your PR since the last changes on master.
Can you check everything seems ok for you ?
Do you think it is possible to disable the button Paste all if nothing in the clipboard ?

Test fails because of two issues with Sylius :

kgonella added 3 commits May 10, 2024 09:13
@kgonella
Enable paste all buttons only if there is something copied before
41cab18
@kgonella
Merge remote-tracking branch 'refs/remotes/origin/feature/copy-paste-…
8ac475c 
…all-contents' into feature/copy-paste-all-contents

# Conflicts:
#	assets/js/app.js
#	src/Resources/public/css/rich-editor-css.css
#	src/Resources/public/js/rich-editor-js.js
#	src/Resources/views/Admin/app.html.twig
@kgonella
Fix bad merged
9eba035
@maximehuran maximehuran merged commit 7655fbe into monsieurbiz:master May 25, 2024
8 of 10 checks passed
@maximehuran
Copy link
Member

Thank you @kgonella 🚀

@kgonella kgonella deleted the feature/copy-paste-all-contents branch July 10, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacquesbh jacquesbh jacquesbh left review comments

@maximehuran maximehuran Awaiting requested review from maximehuran

@delyriand delyriand Awaiting requested review from delyriand

Assignees
No one assigned
Labels
Status: needs review Status: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kgonella @maximehuran @delyriand @jacquesbh @madamebiz