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

[FIX] web_editor: properly handle nested lists and list conversion #32709

Closed

Conversation

Zynton
Copy link
Contributor

@Zynton Zynton commented Apr 16, 2019

These are various fixes for bugs concerning nested lists and the conversion a type of list into an other. Many tests were added to better cover nested lists as well as checklists in general.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@Zynton Zynton requested a review from pparidans April 16, 2019 08:20
@C3POdoo C3POdoo added the RD research & development, internal work label Apr 16, 2019
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from b98852b to aa38f0c Compare April 16, 2019 10:31
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from aa38f0c to ec25c8d Compare April 16, 2019 11:12
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from ec25c8d to 877d698 Compare April 16, 2019 12:19
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 16, 2019
Copy link
Contributor

@pparidans pparidans left a comment

Choose a reason for hiding this comment

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

Looks globally good but... could you move the corresponding tests to each "FIX" commit (instead of having one big "tests" commit).

addons/web_editor/static/src/js/wysiwyg/plugin/helper.js Outdated Show resolved Hide resolved
addons/web_editor/static/src/js/wysiwyg/plugin/helper.js Outdated Show resolved Hide resolved
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from 877d698 to 0ae253d Compare April 23, 2019 08:31
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 23, 2019
@Zynton
Copy link
Contributor Author

Zynton commented Apr 23, 2019

@pparidans Thanks for the review. I applied your requested changes.

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 23, 2019
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from 0ae253d to 647f25a Compare April 25, 2019 09:56
@Zynton
Copy link
Contributor Author

Zynton commented Apr 25, 2019

@pparidans Thanks, all fixed!

@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 25, 2019
The `o_checklist` class was preserved when converting a checklist to an
ordered list.
This addresses various bugs that occured with nested lists - including
lists that are nested with a UL within a LI (as opposed to a UL directly
within another UL).
A list item with text can be written as <li>text</li> or as
<li><p>text</p></li>. Issues occurred when both syntaxes need to be
merged together. Namely, the <p>text</p> was being appended to the other
list item, thereby creating a visible new line.
The "comparisons" website snippet uses a list group with the class set
on a UL. In consequence, the web editor considers it as a regular list
and may try to outdent it, which is not the expected behavior with a
list group. This fix ensures that ULs with the `list-group` class cannot
be in/outdented.
@Zynton Zynton force-pushed the saas-12.2-summernote-convert-lists-age branch from 647f25a to dfa9c58 Compare April 25, 2019 10:07
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 25, 2019
@pparidans
Copy link
Contributor

@robodoo r+

@robodoo
Copy link
Contributor

robodoo commented Apr 29, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 CI 🤖 Robodoo has seen passing statuses labels Apr 30, 2019
@robodoo
Copy link
Contributor

robodoo commented Apr 30, 2019

Staging failed: ci/runbot on 3f8dc210740391c34e63cb9546b9cb78aa828228 (view more at http://runbot.odoo.com/runbot/build/509875)

@pparidans
Copy link
Contributor

@robodoo retry

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 labels Apr 30, 2019
robodoo pushed a commit that referenced this pull request Apr 30, 2019
The "comparisons" website snippet uses a list group with the class set
on a UL. In consequence, the web editor considers it as a regular list
and may try to outdent it, which is not the expected behavior with a
list group. This fix ensures that ULs with the `list-group` class cannot
be in/outdented.

closes #32709

Signed-off-by: Pierre Paridans <pparidans@users.noreply.github.com>
@robodoo robodoo added merging 👷 and removed r+ 👌 CI 🤖 Robodoo has seen passing statuses labels Apr 30, 2019
@robodoo
Copy link
Contributor

robodoo commented Apr 30, 2019

Staging failed: ci/runbot on 48b9a42dd0f444f717bba0c9f641f026c9a07428 (view more at http://runbot.odoo.com/runbot/build/509989)

@pparidans
Copy link
Contributor

@robodoo retry

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 labels May 1, 2019
@robodoo
Copy link
Contributor

robodoo commented May 1, 2019

Merged, thanks!

@robodoo robodoo closed this May 1, 2019
@pparidans pparidans deleted the saas-12.2-summernote-convert-lists-age branch May 2, 2019 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants