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-116201 Update liferay-ckeditor version in portal and implement changes to make it fit with new skin #66
LPS-116201 Update liferay-ckeditor version in portal and implement changes to make it fit with new skin #66
Conversation
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:sf - 0 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-116201-moono-lexicon 1 Failed Jobs:For more details click here.
|
ci:test:sf |
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-116201-moono-lexicon 1 Successful Jobs:For more details click here. |
@jbalsas should we wait until GA4 is released before merging this? |
ci:test:relevant |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6493 |
As per Slack thread, we might want to merge liferay/liferay-ckeditor#78 and cut a release so as to avoid adding to the dependency foot-print. (At least, it's my understanding that the new deps added to liferay-ckeditor should be build-only deps.) |
Currently targeting Thursday for release, so I'd say it is quite reasonable to wait a couple of days if you feel like there is some risk involved here. FWIW, if you've manually tested it in a few places and checked IE, then it is probably ok. |
@@ -60,7 +60,8 @@ public void populateConfigJSONObject( | |||
HtmlUtil.escape( | |||
PortalUtil.getStaticResourceURL( | |||
themeDisplay.getRequest(), | |||
"/o/frontend-css-web/main.css"))) | |||
"/o/frontend-editor-ckeditor-web/ckeditor/skins" + |
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.
Is this really needed? I'd expect CKEditor to load this automatically when passing in a different skin?
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.
Checked, we don't need this :D
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.
Did we set the skin to be the default in our build process in liferay-ckeditor
?
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.
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.
Based on the changes, I think we're failing to properly set the skin through configuration.
My guess is that we're still loading moono.css
and now moono-lexicon.css
on top. We should set config
option to moono-lexicon
and let CKEditor do the rest.
Could we double-check this?
modules/yarn.lock
Outdated
@@ -5324,6 +5333,11 @@ chownr@^1.0.1, chownr@^1.1.1, chownr@^1.1.2: | |||
resolved "https://registry.yarnpkg.com/chownr/-/chownr-1.1.3.tgz#42d837d5239688d55f303003a508230fa6727142" | |||
integrity sha512-i70fVHhmV3DtTl6nqvZOnIjbY0Pe4kAUjwHj8z0zAdgBtYrJyYwLKCCuRBQ5ppkyL0AkN7HKRnETdmdp1zqNXw== | |||
|
|||
chownr@^2.0.0: |
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.
👀
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.
Removed in last commit
I'm not sure the last CI error is related to our change, but if @john-co can confirm that would be great |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6151 |
ci:test:relevant |
Running it again, just in case... @john-co, could you please do some manual smoke tests on this, just to be sure? |
It looks like the same error is coming from tests ran on test-1-17 similar to #90 (comment). test-1-17 is now taken offline, and I think it shouldn't affect the current pending run. |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6487 |
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: |
All required test suite(s) passed. |
Pull request has been successfully forwarded to brianchandotcom#91140 |
https://issues.liferay.com/browse/LPS-116201
Updating
liferay-ckeditor
version in portal in order to apply the new skin we created following Lexicon's specs.Test plan: