Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

IBX-5981: Fixed handling multiple custom classes #95

Merged
merged 2 commits into from
Jun 29, 2023
Merged

IBX-5981: Fixed handling multiple custom classes #95

merged 2 commits into from
Jun 29, 2023

Conversation

dew326
Copy link
Contributor

@dew326 dew326 commented Jun 22, 2023 •
edited
Loading

Question Answer
JIRA issue https://issues.ibexa.co/browse/IBX-5981
Bug/Improvement yes
New feature no
Target version 4.5
BC breaks no
Tests pass yes
Doc needed no

Requires: ibexa/admin-ui#828

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@dew326 dew326 self-assigned this Jun 22, 2023
@micszo micszo self-assigned this Jun 22, 2023
tischsoic
tischsoic approved these changes Jun 22, 2023
View reviewed changes
src/bundle/Resources/public/js/CKEditor/custom-attributes/ui/custom-attributes-form-view.js Outdated
const value = config.multiple
? [...new Set([...labeledDropdown.fieldView.element.value.split(','), event.source.value])].join(',')
: event.source.value;
const alreadySelectedValues = labeledDropdown.fieldView.element.value ? labeledDropdown.fieldView.element.value.split(',') : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can use optional chaining?

Suggested change
const alreadySelectedValues = labeledDropdown.fieldView.element.value ? labeledDropdown.fieldView.element.value.split(',') : [];
const alreadySelectedValues = labeledDropdown.fieldView.element.value.split?.(',') ?? [];

micszo
micszo reviewed Jun 22, 2023
View reviewed changes
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

There is a visual issue without default value before choosing first value.

Screenshot 2023-06-22 at 15 34 12

@micszo
Copy link
Contributor

micszo commented Jun 22, 2023

When using following config

                        paragraph:
                            choices: [ regular, special, tip_box, warning_box ]
#                            default_value: regular
                            required: false
                            multiple: false
                        heading:
                            choices: [ foo, bar ]
#                            default_value: foo
                            required: false
                            multiple: true
                        heading2:
                            choices: [ titre1, titre2, titre3 ]
#                            default_value: titre1
                            required: true
                            multiple: false
                        heading3:
                            choices: [ test1, test2, test3 ]
#                            default_value: test1
                            required: true
                            multiple: true

values are being carried over between classes:

Screenshot 2023-06-22 at 15 48 21

@micszo
Copy link
Contributor

micszo commented Jun 22, 2023

Console error occurs when saving custom attributes:

Screenshot 2023-06-22 at 15 41 18

micszo
micszo requested changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Please see comments above.

@micszo
Copy link
Contributor

micszo commented Jun 23, 2023 •
edited
Loading

Additional feedback from customer: https://jsd.ez.no/browse/CS-11138?focusedCommentId=276759&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-276759

Confirmed commas are present in page source.

@dew326 dew326 force-pushed the fix-heading-multiple branch from e5e9df1 to 4101ce1 Compare June 28, 2023 12:15
@dew326
Copy link
Contributor Author

dew326 commented Jun 28, 2023

@micszo all issues should be fixed. Please note there is another PR ibexa/admin-ui#828

@sonarqubecloud
Copy link

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

@dew326 dew326 requested a review from micszo June 28, 2023 12:17
micszo
micszo approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all the issues!

QA Approved on Ibexa Experience 4.5.1-dev.

@micszo micszo removed their assignment Jun 29, 2023
@micszo micszo mentioned this pull request Jun 29, 2023
Merged
2 tasks
@dew326 dew326 merged commit c223ef0 into 4.5 Jun 29, 2023
@dew326 dew326 deleted the fix-heading-multiple branch June 29, 2023 14:11
@dew326 dew326 mentioned this pull request Jul 12, 2023
Merged
4 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Gengar-i Gengar-i Gengar-i approved these changes

@lucasOsti lucasOsti lucasOsti approved these changes

@micszo micszo micszo approved these changes

@tischsoic tischsoic tischsoic approved these changes

@GrabowskiM GrabowskiM GrabowskiM approved these changes

Assignees

@alongosz alongosz

@dew326 dew326

Labels
QA approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dew326 @micszo @GrabowskiM @tischsoic @lucasOsti @Gengar-i @alongosz