-
Notifications
You must be signed in to change notification settings - Fork 27
Replace Froala with Quill #8739
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
Replace Froala with Quill #8739
Conversation
✅ Deploy Preview for ilios-frontend ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dartajax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "undo" button should not be activated unless the user has made a change.
|
@dartajax the blue background and undo/redo issues should be fixed. @jrjohnson I also threw in the "hide insert link popup if you click away from it" fix. |
jrjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/ilios-common/addon-test-support/ilios-common/helpers/quill-editor.js
Outdated
Show resolved
Hide resolved
| } | ||
| export async function quillEditorValue(element) { | ||
| const editor = await getEditorInstance(element); | ||
| return editor.root.innerHTML.split(' ').join(' ').replaceAll('<p><br></p>', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a comment or link. I'm guessing this is the only way to get at the value here, but it seems weird to reach into the HTML and that there isn't an API in quill to get the current value of the editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a call to get the contents: https://quilljs.com/docs/api#getcontents, but it doesn't work as well as what I used.
There are three ways to get at what's in the editor (I had just added the exclamation point at the end):

| if (!this.isDestroyed && !this.isDestroying) { | ||
| this.editorHasNoRedo = !this.editor.history.stack.redo.length; | ||
| this.editorHasNoUndo = !this.editor.history.stack.undo.length; | ||
| // make sure to retain multiple spaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is what the code does, but maybe a deeper comment on why is needed? Why not just let quill collapse spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this code is because Froala also retains multiple spaces, but Quill needs this extra bit to achieve parity.
| export async function loadQuillEditor() { | ||
| const { default: QuillEditor } = await import('quill'); | ||
|
|
||
| // Quill doesn't include redo/undo icons by default, so gotta hack 'em in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use what's in font awesome for this? I'm not worried about style yet, but I don't love carrying some random SVG stuff here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried Fontawesome but the icons don't match the rest of the toolbar buttons.
The <svg> markup is not random, as it comes from Quill itself: node_modules/quill/assets/icons/[undo|redo].svg. Why this isn't wired up already, I do not know (nor does anyone else using Quill that I could find). Perhaps an upstream PR is needed?
I put a comment in the code about this, and reproduced the link I followed here so you know where this method came from: slab/quill#885 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, how about we pull this svg out of node_modules and into /public and use it directly? My objection is just to the svg in the code that's a little bit hard to figure out where it's from or when to update it.
b6a3c7f to
13aadd4
Compare
|
@jrjohnson Editor instance now fills container, and link popup closes on escape key. Froala doesn't currently have an 'X'-type close button, so I didn't think to add it to Quill. However, it could be added. |
1f80596 to
7caa346
Compare
|
@jrjohnson The best thing I came up with so far for loading the redo/undo svg files is here 1f80596 |
7caa346 to
ebb89eb
Compare
|
I'm good with the |
d9af9eb to
5119b38
Compare
stopfstedt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code reviewed and click-tested. LGTM.
dartajax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
https://deploy-preview-8739--ilios-frontend.netlify.app/courses/2318/sessions/nytimes.com I inserted the link "nytimes.com" into a session description field. The URL above will always get you zoinked. |
|
@dartajax I added in some logic to hopefully turn protocol-less links like that into protocol-rich links. |
|
@dartajax the "Open in new tab" option was initially put in there, unchanging, because Quill doesn't have a mechanism to put a link in without opening a new tab/window. However, I wanted to make the dialog with as much parity to Froala as possible. That being said, I could spend more time trying to reverse engineer their logic and make that toggle work, or I could just remove it entirely and the User Guide could mention the change from Froala to Quill. |
f51ae84 to
23ca15e
Compare
|
Look into adding the link target toggle functionality. |
|
@dartajax I added the functionality to make a link open in a new tab or not, so that's taken care of. Please give it a whirl to make sure it works. Still need to add tests, though. |
dartajax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave this one as a comment rather than a change request although it may end up being that as well.
There may be a few of these also as I have noticed some issues.
This is in Froala ...
After clicking the edit link (as shown above), you get the following ...
With Quill what happens? ...
Click edit as shown above ...
The only thing that can be edited is the link itself and not whether it opens in a new tab or what the text display value is. Froala allows for editing of this information.
But it gets worse or weirder. When you click "Save" ...
After clickiing "Save" as shown above ...
Return to the link and see this ...
It is true if you cancel the operation and don't save any changes, the original link will still be there but there may be other text changes that needed to be made and that should not cause the link to be erased.
…links into editor
… that editinplace was showing
…t figure out how to access quill internal object
…r the Virtual Learning Link style
…more like offering save method
…b for proper space encoding
…ked valid (enough)
…tack and unavailable for undoing
…to history after user stops typing
… tests dont break
|
@dartajax Do you want me to work further on the Word document copy/paste minor discrepancy? |
|
This may be unrelated, but I see siteimprove is picking up Role with implied hidden content has keyboard focus on a demo quill edit link when it's within our expanding text widget. I'm wondering if something about the HTML output is slightly different and now triggering an error where we haven't had one before. |
3774c2f to
1b47120
Compare





Fixes ilios/ilios#6353
The Quill is mightier than the Froala, or something.
Still have a hojillion tests to fix, but this is a Proof of Concept that works pretty well as a replacement. The only thing I still need to add, feature-wise, is a more dynamic, local CSS load (it's currently global on the
index.htmlfile), and the pre-chosen links for Insert Link.Feel free to mess around with it and find where it breaks!