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

IBX-3988: RichText formatting options not stored for Numbered List and Bulleted List #64

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

vidarl
Copy link
Contributor

@vidarl vidarl commented Jan 19, 2023

Question Answer
JIRA issue IBX-3988
Bug/Improvement yes
New feature yes
Target version 4.3
BC breaks no
Tests pass yes/no
Doc needed yes/no

This PR ensures that styling in lists are stored in docbook

Related DocBook Specs:
https://tdg.docbook.org/tdg/5.0/orderedlist.html
https://tdg.docbook.org/tdg/5.0/itemizedlist.html

TODO:

  • Remove decimal-leading-zero button from Editor - It is not supported by DocBook Added support for decimal-leading-zero in our schema instead
  • Implemented XHTML output conversion
  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@vidarl vidarl changed the title Ibx 3988 styled list WIP: IBX-3988 RichText formatting options not stored for Numbered List and Bulleted List Jan 19, 2023
@vidarl
Copy link
Contributor Author

vidarl commented Jan 19, 2023

I discussed this briefly with Andrew today. I tried to replace all the ifs with a mapping in XSLT but that halted due to bugs in libxml2 ( shown to Andrew ). I anyway think this is faster and cleaner than creating a separated PHP converter for doing the job

@vidarl
Copy link
Contributor Author

vidarl commented Jan 19, 2023

FYI : Failing solr tests cannot be my doing ....

@vidarl
Copy link
Contributor Author

vidarl commented Jan 19, 2023

Maybe @dew326 or anyone could comment on how to easily disable decimal-leading-zero button in editor ?

edit:
Or @alongosz : do you want me to add that possibility in our schema ?

@dew326
Copy link
Contributor

dew326 commented Jan 20, 2023

Unfortunately, there is no simple way to remove this one list (CKEditor doesn't provide a configuration for that). It would require overriding the whole numberedList component.

@vidarl
Copy link
Contributor Author

vidarl commented Jan 24, 2023

Unfortunately, there is no simple way to remove this one list (CKEditor doesn't provide a configuration for that). It would require overriding the whole numberedList component.

That is really pity. So, for now I have added support for decimal-leading-zero in our schema.. Are we all okay with this ?
See 0ed670b

@vidarl vidarl changed the title WIP: IBX-3988 RichText formatting options not stored for Numbered List and Bulleted List IBX-3988 RichText formatting options not stored for Numbered List and Bulleted List Jan 24, 2023
@vidarl vidarl requested a review from alongosz January 24, 2023 08:28
@ViniTou ViniTou changed the title IBX-3988 RichText formatting options not stored for Numbered List and Bulleted List IBX-3988: RichText formatting options not stored for Numbered List and Bulleted List Feb 2, 2023
@alongosz
Copy link
Member

alongosz commented Feb 2, 2023

@vidarl CI is failing here

@bogusez bogusez self-assigned this Feb 3, 2023
@vidarl
Copy link
Contributor Author

vidarl commented Feb 3, 2023

FYI : As a test I just reverted all my changes and CI is still failing with same errors.. https://github.com/ibexa/fieldtype-richtext/actions/runs/4084236089/jobs/7040701035

@sonarcloud
Copy link

sonarcloud bot commented Feb 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alongosz alongosz merged commit 3711ae5 into 4.3 Feb 3, 2023
@alongosz alongosz deleted the ibx-3988-styled-lists branch February 3, 2023 13:31
@alongosz
Copy link
Member

alongosz commented Feb 3, 2023

Thank you @vidarl 🎉

@bogusez
Copy link

bogusez commented Feb 6, 2023

@vidarl we have a bug on main branch. This is weird because on v4.3.2 (with .diff) and on v4.3.x-dev this bug does not exist.
The problem is with 'Bulleted List'. For some reason when I click on 'Bulleted List' it does not select the default value which is 'Disc' but select 'Circle'.
Please have a look on gif: https://recordit.co/2FXdBMxVqR

As you can see on the gif, selected 'Bulleted List' option on create and edit form is not correct.

Tested on clean installation of commerce 4.4.x-dev

P.S.
'Numbered List' works correctly.

@vidarl
Copy link
Contributor Author

vidarl commented Feb 6, 2023

So, you only have problems with commerce ? or on any 4.4 ?
This patch is not supposed to do any changes in the behavior when you click on the bullet list button ( only when you click on the drop down to select a particular bullet list style ).

So it could be that commerce behaves like this both with and without this patch maybe ?

@bogusez
Copy link

bogusez commented Feb 6, 2023

So, you only have problems with commerce ? or on any 4.4 ? This patch is not supposed to do any changes in the behavior when you click on the bullet list button ( only when you click on the drop down to select a particular bullet list style ).

So it could be that commerce behaves like this both with and without this patch maybe ?

I will check it on Exp and Content versions as well as on v4.3.2 commerce (without the patch). Will get back to you with informations.

@bogusez
Copy link

bogusez commented Feb 6, 2023

Described by me problem does not occur on any edition of tag v4.3.2. I guess the problem as you mentioned is not related to your changes. I will report it in a separate task. Issue occurs on all editions (oss, content, exp and commerce) on branch 4.4.x-dev.

@bogusez
Copy link

bogusez commented Feb 7, 2023

Issue reported here: https://issues.ibexa.co/browse/IBX-5030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants