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

fix: check if function is available #1554

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

fortunatomaldonado
Copy link
Member

https://liferay.atlassian.net/browse/LPP-50808
#1553

After upgrading CKEditor, there are errors when trying to create a blog. This is due to CKEditor no longer providing the detach methods for these two instances.

I added conditions to check if the methods exist to resolve this issue on the upgrade and lower versions.
Let me know if there are any questions or comments about this.
Thank you!

@deborasoliveira
Copy link

ci:test:relevant

@@ -229,7 +229,10 @@ class UI extends React.Component {
}

if (this._keyDownListener) {
this._keyDownListener.detach();
if (typeof this._keyDownListener.detach === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Is this._keyDownListener.detach possible to be anything other than a function or falsy?

If we just need to check if it exists, we can just do if (this._keyDownListener.detach) {

@@ -153,7 +153,9 @@ if (!CKEDITOR.plugins.get('ae_uicore')) {
editor.on('destroy', _event => {
ariaElement.parentNode.removeChild(ariaElement);

handleUI.detach();
if (typeof handleUI.detach === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@fortunatomaldonado
Copy link
Member Author

Hello @bryceosterhaus ,
I updated the conditions and pushed the changes.
Let me know if there is anything else needed.
Thank you!

@bryceosterhaus
Copy link
Member

Looks good!

btw I have no experience releasing alloy-editor. Let's see if @markocikos knows the next steps, looks like he released last year

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

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

@markocikos markocikos merged commit fd58d1a into liferay:master Oct 20, 2023
@markocikos
Copy link
Member

@fortunatomaldonado Please see instruction for release in https://github.com/liferay/alloy-editor/wiki/Contributing. The release process here is similar to what we did with CKEditor, but simpler, since there are no patches here. You should be able to make the release. Let me know if you get stuck somewhere, and I'll complete it.

@fortunatomaldonado
Copy link
Member Author

Hello @markocikos, I am having issues again with the npx liferay-changelog-generator --version=$VERSION command just like in CKEditor.
Getting the following error: Error: Expected to run from repo root () or "packages/*" but was run from C:\Users\Liferay\Desktop\Repos\alloy-editor

I am on Windows and I think this command doesn't like being ran on Windows, or I might be doing something wrong. I'll see what I can do to resolve this for future times.

Can you complete the release?
Thank you!

@markocikos
Copy link
Member

@fortunatomaldonado I released v2.14.10 to npm. Please go ahead and update version in DXP.

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

Successfully merging this pull request may close these issues.

None yet

4 participants