-
Notifications
You must be signed in to change notification settings - Fork 1
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
LPS-117468 Uses moono-lisa skin #175
Conversation
…lexicon since PageEditor uses AlloyEditor
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
ci:test:sf |
ci:test:relevant |
✔️ ci:test:sf - 1 out of 1 jobs passed in 2 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-117468 1 Successful Jobs:For more details click here. |
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.
It's curious to me that we need to specify skin in usage, I would expect this to be inherited from BaseAlloyEditorConfigContributor. Why do we allow editors like fragment editor to extend BaseEditorConfigContributor
? I would expect we allow extension of either BaseAlloyEditorConfigContributor
or BaseCKEditorConfigContributor
. There are 32 results for extends BaseEditorConfigContributor
, all of them will have no default configuration, including no skin.
The change is a good workaround for now, it fixes the issue, but we should consider restricting our allowed configuration inheritance. Devs should start with an alloyeditor
and modify it, instead of starting from zero and modify it to something close enough to alloyeditor
to use its resources and skin.
Would it be an option to set the skin in |
Hey @markocikos, @carloslancha, thanks for the feedback!
It wouldn't, since
Yeah, this makes total sense... as I said here, I don't think it actually makes sense to be able to use any skin whatsoever with AlloyEditor, but wanted to get this fixed for GA5. I'll move the configuration to the |
Ok, scratch that 😂 Not every So, I'd say this is the way to go:
Please, @markocikos, @carloslancha, do you mind double-checking on this and let me know what you think? Forward if satisfied with my empty promises :) |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6890 |
Yeah, I agree we should merge this. ✔️ What I suggest as a no-rush task is to change:
to
... and to modify the configuration to override |
True, didn't think about that.
We're setting |
ci:test:relevant |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
Skipping previously passed test suites: |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6229 |
All required test suite(s) passed. |
Pull request has been successfully forwarded to brianchandotcom#91914 |
Jenkins Build:test-portal-source-format#4322 |
**Changes in this version** [Full changelog](liferay/liferay-ckeditor@v4.16.0-liferay.1...v4.16.0-liferay.2) ### 🔧 Bug fixes - fix: remove unneeded styles ([\#176](liferay/liferay-ckeditor#176)) - fix: update @clayui/css to latest and add more icons ([\#175](liferay/liferay-ckeditor#175)) - fix: add null check ([\#168](liferay/liferay-ckeditor#168))
By default,
liferay-ckeditor
now uses themoono-lexicon
skin that better matches DXP's look and feel. That skin, however, is not meant to be used withAlloyEditor
and introduces some conflicting styling.This PR changes the skin used inside PageEditor AlloyEditor instances back to
moono-lisa
.To test:
@p2kmgcl, can you verify this addresses your issue?
PS: This is a followup of #157