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

Added the image resize balloon #16140

Merged
merged 36 commits into from
Apr 15, 2024
Merged

Added the image resize balloon #16140

merged 36 commits into from
Apr 15, 2024

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Apr 2, 2024

Suggested merge commit message (convention)

Feature (image): Add custom image width input option in the image editing toolbar, as an alternative to drag-and-drop resizing


Additional information

Related to https://github.com/cksource/ckeditor5-commercial/issues/6023
Related to #16104
Screens:
obraz
obraz

@Mati365 Mati365 marked this pull request as ready for review April 3, 2024 07:20
@Mati365 Mati365 requested a review from scofalik April 3, 2024 17:22
@oleq oleq changed the title Add resize image column balloon editor Added the image resize balloon Apr 9, 2024
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

  1. You can submit an invalid value. I think we need to validate these for empty values just like links or media
Screen.Recording.2024-04-09.at.14.50.02.mov
  1. When in px, there's a min value limit set to 50px. This is no good because we allow for resizing inline images too that are often used as emoji or just really small images (see the docs). They go way below 50px limit.
image image
  1. When in px, the upper limit of 2400 is also problematic. You can easily use a wider hero image in HiDPI than 2400 (or especially for print, export to PDF).
image
  1. There's something odd going on with units and I spotted it in the picture manual test. The input label is in % but the value is clearly in px (put 100 there and it will commit as 100%, though)
image

@Mati365
Copy link
Member Author

Mati365 commented Apr 9, 2024

@oleq Good catch, will be fixed. I'll change a bit min / max values.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

I also spotted some layout issue with narrow viewports

image

@Mati365 Mati365 requested a review from oleq April 11, 2024 04:59
@Mati365
Copy link
Member Author

Mati365 commented Apr 11, 2024

@oleq All reported CR remarks fixed.

@oleq
Copy link
Member

oleq commented Apr 11, 2024

Two more problems that I noticed:

  1. After recent changes, the input got squashed.
  2. When the error shows up, the balloon stops pointing to the image (probably we need editor.ui.update() call)
Screen.Recording.2024-04-11.at.14.34.35.mov

@Mati365
Copy link
Member Author

Mati365 commented Apr 11, 2024

@oleq Fixed

@Mati365
Copy link
Member Author

Mati365 commented Apr 12, 2024

@oleq I fixed all CR remarks.

@charlttsie
Copy link
Contributor

charlttsie commented Apr 12, 2024

Inconsistencies when it comes to the minimal value that can be inserted

Steps

  1. Open http://localhost:8125/ckeditor5-image/tests/manual/imageresize.html
  2. Try to insert 0,1 as a custom image size
  3. Resize an inline image to the smallest possible size using resize handler
  4. Resize an inline image to the smallest possible size using resize handler

Results

The minimal value that can be inserted is 0,2 - both for inline and block images:

resize-minimal.mp4

However, an inline image can be made smaller using the resize handler:
inline-image

And at the same time, a block image can't be made that small and incorrect values are shown from a certain point:

image-block.mp4

@charlttsie
Copy link
Contributor

Resize balloon shows a wrong maximum image size in pixels

Steps

  1. Open http://localhost:8125/ckeditor5-image/tests/manual/imageresizepx.html
  2. Enlarge an image to the maximum possible size using resize balloon
  3. Enlarge an image to the maximum possible size using resize handler

Result

Resize balloon allows for setting the size to 1600 px, while the image has only 1548 px at its maximum size and this is what's shown after resize handler is used:

max-size.mp4

@Mati365
Copy link
Member Author

Mati365 commented Apr 12, 2024

@charlttsie Please pull, some modifications regarding image resize range have been pushed.

@Mati365 Mati365 requested a review from oleq April 12, 2024 10:34
@charlttsie
Copy link
Contributor

@Mati365
#16140 (comment) looks good for inline images.
For block images, the limit looks good now:
block-1
However, the field can still show an incorrect value after using resize handler:
resize-block
This looks related to an issue that's also reproducible on master, where from a certain point when resizing, the actual image size doesn't change anymore yet the number is still changing:

percent.mp4

So, I'm not sure if that's within the scope 🤔

@Mati365
Copy link
Member Author

Mati365 commented Apr 12, 2024

@charlttsie It's out of scope, this is not related to this particular implementation (as it just works identically as "draggable" one). Anyway - thanks for report.

@charlttsie
Copy link
Contributor

Regarding #16140 (comment), when I take the same image from http://localhost:8125/ckeditor5-image/tests/manual/imageresizepx.html and resize it using resize handler, the size shown is 1548 px and it also shows inside the resize balloon field. However, when I want to submit it, it won't get accepted:

max-size.mp4

@charlttsie
Copy link
Contributor

#16140 (comment) looks good 👍

@charlttsie
Copy link
Contributor

FYI: I reported the issue with the incorrect size when shrinking a block image here: #16208

@charlttsie
Copy link
Contributor

We've finished testing with @kubaklatt and @mabryl and besides the already reported and fixed issues, the PR looks good. Thank you @Mati365 for the quick fixes 🎉

@Mati365
Copy link
Member Author

Mati365 commented Apr 15, 2024

@oleq UI update test added.

@oleq oleq merged commit 7c0d752 into master Apr 15, 2024
1 check was pending
@oleq oleq deleted the ck/6023-image branch April 15, 2024 12:03
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

3 participants