-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Admin, add trix editor to product description editor (both new and edit) #11140
Admin, add trix editor to product description editor (both new and edit) #11140
Conversation
dd80f3e
to
c04f29a
Compare
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.
Nice work! ✨
c04f29a
to
e413a16
Compare
this.#trixInitialize(); | ||
window.addEventListener("trix-initialize", this.#trixInitialize); |
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.
Do we need both line here ? You would think the listener on trix-initialize
would be enough ?
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 thought so, and that's the normal way of doing it. Unfortunately, for some reasons, trix-initialize
can be triggered before the stimulus controller is connected to the trixeditor. But, sometimes, that's the opposite. That's why I've added the two ways....
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.
Tricky one, I can't think of any other solution 🤷♂️
Hi @jibees, |
f3e49af
to
e05633a
Compare
I resolved the conflicts. |
4fce3ff
to
b970339
Compare
I resolved the (others ones) conflicts. |
Hi @jibees, Before stagingAfter stagingThere are a few little things we should fix before merging. First of all we should update the page /admin/products/new at the same time. Otherwise we might end up in a mess and it was requested in the issue: #11129 (comment). In the shopfront:
In the editor: The good thing is: Previously formatted descriptions are still displayed nicely, just underlined is not supported anymore. 👍 I will move this back to In Dev to fix the last bits. Thanks! 🙏 |
b970339
to
c4d0742
Compare
Thanks @drummer83 |
991adca
to
2ff2a5a
Compare
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 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 like the use of the shared CSS. Thanks.
Hi @jibees, |
2400d9c
to
0884887
Compare
trix doesn't allow the use of `<p>` as block separator since it can not contain `<figure>`: use `<div>` and _emulate_ as `<p>` with margin bottom
Use the TrixSanitizer | TrixScrubber
Will be used elsewhere (in shopfront in particular)
0884887
to
b6b64e9
Compare
@drummer83 |
Thanks, @jibees! Here we go again. Shopfront ✔️Product modal ✔️Edit product ✔️Create new product ✔️Newly created product ✔️Custom tab ✔️After: Lists ❓Before: After: ConclusionGreat! This seems to work very well now! 🎉
Those things are out of scope here and this is ready to merge! 🚀 |
What? Why?
Also, I've factorized custom tab, in order to have common rules for both shopfront product modal and custom tab.
New
Editing
Product modal in shopfront
Custom tab edition
Custom tab in shopfront
What should we test?
We should test that htmlized product description is still well designed, so we need to test before and after content has been modified by shop manager.
Before staging the PR
After staging the PR
Test that the custom tab edition (in admin) is accurate to custom tab viewing (in shopfront)
Release notes
Changelog Category: User facing changes