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-769: Reorder and hide for CKEditor toolbar groups and buttons, CustomStyleInline button support #203

Merged
merged 9 commits into from
Oct 13, 2021

Conversation

damianz5
Copy link
Contributor

@damianz5 damianz5 commented Aug 18, 2021

Question Answer
JIRA issue IBX-769
Improvement yes
New feature yes
Target version master
BC breaks yes
Tests pass yes
Doc needed yes
  • Reorder and hide for CKEditor toolbar groups and buttons,
  • show CustomStyleInline button only if inline custom styles exists,

TODO:

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

@SerheyDolgushev
Copy link
Contributor

It seems like a nice idea. Right now we are using basic yaml syntax to achieve the same results:

ezplatform:
    system:
        admin_group:
            fieldtypes:
                ezrichtext:
                    toolbars:
                        custom_tag1: &custom-tag-common-toolbar
                            buttons:
                                ezmoveup:
                                    priority: 80
                                ezmovedown:
                                    priority: 70
                                ezcustomtagedit:
                                    priority: 60
                                ezanchor:
                                    priority: 50
                                ezembedleft:
                                    priority: 40
                                ezembedcenter:
                                    priority: 30
                                ezembedright:
                                    priority: 20
                                ezblockremove:
                                    priority: 10
                        custom_tag2: *custom-tag-common-toolbar
                        custom_tag3: *custom-tag-common-toolbar
                        custom_tag4: *custom-tag-common-toolbar

Maybe it worth to add this example in documentations?

@damianz5 damianz5 marked this pull request as ready for review August 31, 2021 23:47
@damianz5 damianz5 requested a review from a team September 1, 2021 08:02
@damianz5 damianz5 requested a review from Steveb-p September 1, 2021 09:37
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

PR description declares that this does not introduce BC breaks. Can you confirm that users having previous configuration variant can safely upgrade without changing it?

What about existing editor extensions? Do they need to be updated?

I see that this goes into master branch, but if it introduces BC breaks in any way, then they should be documented and an upgrade path has to be provided.

src/lib/Configuration/Provider/CKEditor.php Outdated Show resolved Hide resolved
tests/lib/Configuration/Provider/CKEditorTest.php Outdated Show resolved Hide resolved
tests/lib/Configuration/Provider/CKEditorTest.php Outdated Show resolved Hide resolved
@damianz5 damianz5 requested a review from Steveb-p September 6, 2021 21:11
@damianz5 damianz5 force-pushed the IBX-769-extensibility-for-online-editor branch from 1e153d6 to d9c332e Compare September 24, 2021 09:33
@damianz5 damianz5 requested a review from a team September 24, 2021 09:36
@alongosz
Copy link
Member

@damianz5 I feel like both JIRA issue and the title is to vague. Is this about extensibility of toolbar buttons or something more? Specifically what does it mean "Compatible with Custom plugins"?

@alongosz alongosz self-assigned this Sep 24, 2021
@damianz5 damianz5 force-pushed the IBX-769-extensibility-for-online-editor branch from d9c332e to 1209de7 Compare September 24, 2021 11:43
@damianz5 damianz5 changed the title IBX-769: Extensibility for Online Editor IBX-769: Reorder and hide for CKEditor toolbar groups and buttons, CustomStyleInline button support Sep 24, 2021
@damianz5
Copy link
Contributor Author

PR description updated

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Looks ok for me, although I'm not a fan of prefixing calls to global functions with \. Performance improvement is negligible (especially with OPCache).

@Steveb-p Steveb-p requested a review from a team October 4, 2021 07:36
@ViniTou ViniTou requested a review from a team October 4, 2021 07:45
->arrayNode(self::TOOLBARS_NODE_KEY)
->useAttributeAsKey('name')
->info('List of Toolbars and Buttons enabled for current SiteAccess scope.')
->arrayNode(self::TOOLBAR_NODE_KEY)
Copy link
Member

Choose a reason for hiding this comment

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

This BC break needs to be mentioned in upgrade notes @ezsystems/documentation-team

@DominikaK DominikaK added the Doc needed The changes require some documentation label Oct 4, 2021
@tomaszszopinski tomaszszopinski self-assigned this Oct 6, 2021
@damianz5 damianz5 force-pushed the IBX-769-extensibility-for-online-editor branch from 43a2b18 to 70f37ca Compare October 11, 2021 08:55
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP 4.0.0-dev.

@damianz5 damianz5 merged commit bac7c8e into master Oct 13, 2021
@damianz5 damianz5 deleted the IBX-769-extensibility-for-online-editor branch October 13, 2021 07:36
@juskora juskora removed the Doc needed The changes require some documentation label Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

10 participants