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

Fixes #1048 | Add button to remove images #1096

Merged
merged 5 commits into from
Feb 14, 2019
Merged

Fixes #1048 | Add button to remove images #1096

merged 5 commits into from
Feb 14, 2019

Conversation

julien
Copy link
Contributor

@julien julien commented Feb 12, 2019

The next part of the fix is to add a button to update
images.

The next part of the fix is to add a button to update
images.
@julien julien requested a review from wincent February 12, 2019 16:39
Copy link
Contributor

@wincent wincent left a comment

Choose a reason for hiding this comment

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

Looks good apart from what looks to be an unnecessary prop?

Also, I noticed that we're doing the addComand call in two different ways in the codebase: one like you're doing here and another with new CKEDITOR.command. As a follow-up might be good to switch them all to use the pattern you're using here (assuming they are actually equivalent).

refactor remove image button
Copy link
Contributor

@wincent wincent left a comment

Choose a reason for hiding this comment

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

Looks good to me once you've added AlloyEditor.Strings.removeImage.

* @inheritDoc
*/
render() {
const cssClass = `ae-button ${this.getStateClasses()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking forward to getting away from this pattern (where we call a method that we don't define on the class instance but inherit magically due to our HOC's use of extends).

@wincent
Copy link
Contributor

wincent commented Feb 14, 2019

Merging this so it doesn't get stale with respect to the parallel changes going on with the language files (eg. #1107).

@wincent wincent merged commit 02c33d5 into liferay:master Feb 14, 2019
@julien julien deleted the issue/1048 branch February 14, 2019 15:09
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.

2 participants