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

i/5201: Add ImageResizeUI plugin. #7482

Merged
merged 26 commits into from
Jul 7, 2020
Merged

i/5201: Add ImageResizeUI plugin. #7482

merged 26 commits into from
Jul 7, 2020

Conversation

panr
Copy link
Contributor

@panr panr commented Jun 23, 2020

Suggested merge commit message (convention)

Feature (image): Introduced the UI for a manual image resizing via a dropdown or standalone buttons depends on the configuration. Closes #5201. Closes #5197.


Additional information

The PR also updates Image plugin configuration API, by adding a new property imageResizeOptions.

@panr
Copy link
Contributor Author

panr commented Jun 23, 2020

This is still WIP:

  • Icon
  • API docs updates
  • Event / command handling in the unit tests...

@panr panr marked this pull request as draft June 23, 2020 07:27
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

The behavior is cool and works well :)

The comments in the code are mostly cosmetic.

UI/UX part

  1. The label alignment in dropdown feels weird to me:

2. Buttons in the toolbar have repeated icons.

I don't have better option for this in my mind but (maybe as a follow up) - do you think that we could have here something less cluttered with duplicated arrows icons. My initial thought on this is some kind of "inner toolbar"

icon btn_original btn_50 btn_75

So it would have always an icon, then configured buttons.

The other idea is to have icon display configurable for those buttons so it would not add them.

ps.: The UI questions are rather hints/suggestions as I don't feel strong in that field.

packages/ckeditor5-image/src/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/imageresizeui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/manual/imageresizeui.html Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/manual/imageresizeui.md Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/imageresizeui.js Outdated Show resolved Hide resolved
@panr
Copy link
Contributor Author

panr commented Jul 3, 2020

@jodator I fixed most of the issues. Still to do:

I'll take care of these on Monday.

@jodator jodator self-assigned this Jul 6, 2020
@panr
Copy link
Contributor Author

panr commented Jul 6, 2020

Regarding the plugin icon... I was thinking about something like this:

image

but just after I implemented it, I'm not so sure anymore... @jodator WDYT?

@jodator
Copy link
Contributor

jodator commented Jul 6, 2020

but just after I implemented it, I'm not so sure anymore... @jodator WDYT?

Kinda OK and kind not OK - but again, I don't think I'm the best one to judge UI ;)

Looks better but it brings new UI pattern - which should be probably discussed. And since we have problem with this maybe let's go with label buttons for now and make the UI refinement in a follow-up. WDYTAT?

@panr
Copy link
Contributor Author

panr commented Jul 6, 2020

Looks better but it brings new UI pattern - which should be probably discussed. And since we have problem with this maybe let's go with label buttons for now and make the UI refinement in a follow-up. WDYTAT?

I'm OK with that 👌

@panr panr marked this pull request as ready for review July 6, 2020 13:59
@panr panr requested a review from jodator July 6, 2020 13:59
@jodator
Copy link
Contributor

jodator commented Jul 7, 2020

One thing slipped:

Plus some cosmetics to the docs and class layout:

  1. The resizeOption is in wrong path (AFAICS should be in image/imageresize/imageresizeui):

  2. And is not referenced in resizeOptions configuration (there's String):

    image

  3. The ImageResizeUI should be in image/imageresize not image/imageresize/ui/ (directory and module).

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

☝️

@panr panr requested a review from jodator July 7, 2020 08:29
@panr
Copy link
Contributor Author

panr commented Jul 7, 2020

Thanks for catching those issues 🙏

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Nice, grab a 🍪

@jodator jodator merged commit 70e0b41 into master Jul 7, 2020
@jodator jodator deleted the i/5201 branch July 7, 2020 09:50
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.

Buttons for setting image width to e.g. 50%, 75%, 100% Restoring the original size of a resized image
2 participants