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

WIP: Add inline image resizing UI #13737

Merged
merged 9 commits into from
Feb 18, 2019
Merged

WIP: Add inline image resizing UI #13737

merged 9 commits into from
Feb 18, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Feb 7, 2019

Description

This PR adds UI to resize inline images. This is a more hidden feature. This PR should aim to add the minimum UI needed so that the image dimensions can be changed without TinyMCE's resize handles.

  • Should PostitionedAtSelection be moved to the rich-text package or the components package?

screenshot 2019-02-07 at 16 49 00

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 7, 2019
@ellatrix ellatrix added Needs Design Needs design efforts. [Package] Format library /packages/format-library labels Feb 7, 2019
@ellatrix
Copy link
Member Author

ellatrix commented Feb 7, 2019

@jasmussen I might need your help with the design of this. :) Ideally it should look a bit like the link input pop up. Should the input have a label? It's otherwise unclear what the input field is for? Feel free to change anything on this PR!

@ellatrix ellatrix requested a review from jasmussen February 7, 2019 15:49
@jasmussen
Copy link
Contributor

Cool!

I pushed a little polish:

screenshot 2019-02-08 at 09 52 44

Is there a chance we can get some resize handles? Or is this the new interface?

@ellatrix
Copy link
Member Author

ellatrix commented Feb 8, 2019

@jasmussen Thanks! The image block has resize handles that we added. Currently the inline images have resize handles that TinyMCE added. I'm trying to add some minimal accessible UI to the inline images when we pull out TinyMCE, so resizing is still possible. As this is a lesser used feature, adding resize handles is not a top priority.

@jasmussen
Copy link
Contributor

Works for me. Shine on!

@ellatrix ellatrix added this to the 5.1 (Gutenberg) milestone Feb 10, 2019
@ellatrix ellatrix force-pushed the try/image-format-resize branch from bd11d48 to 38cb03d Compare February 15, 2019 10:38
@ellatrix ellatrix force-pushed the try/image-format-resize branch from e25927e to dfc532c Compare February 15, 2019 11:28
@ellatrix
Copy link
Member Author

Rebased. Seems to work fine now.

There's a small issue with the image format that is more obvious after this PR. The image object receives boundaries, but it shouldn't, as it's just an object, and at those boundaries, the UI is displayed. This issue as already present in master. I plan to fix it with a refactoring of the object format (don't worry, no API changes). The idea is that the objects are so different from formats that it justifies a separate key in the rich text value.

  • There can only be one per index.
  • We have to add object: true to it for special handling.
  • It should not have format boundaries.
  • I think there are some problems during serialisation where the object should always be last, but after applying a format it isn't.

With a separate key for objects, all these issues would go away. Perhaps something similar should be done for line separator formats (read: lists).

@youknowriad
Copy link
Contributor

Some issues:

  • When I enter the format from the left (keyboard), the width input is empty
  • Why do I have to type "Enter" to apply the change, since the input is directly visible, I expect that the change is applied without submitting the form.
  • Copy pasting text with an inline image, results in a paragraph block and an image block.

@ellatrix
Copy link
Member Author

  • Why do I have to type "Enter" to apply the change, since the input is directly visible, I expect that the change is applied without submitting the form.

It's like the link input. Without enter, the image would continuously change in size.

@ellatrix
Copy link
Member Author

  • Copy pasting text with an inline image, results in a paragraph block and an image block.

This is an issue in master as well. Pasted images rarely occur outside of paragraphs, so they are taken out.

@ellatrix
Copy link
Member Author

  • When I enter the format from the left (keyboard), the width input is empty

I thought this issue would be gone after f96dbe0. Could reproduce before but not after.

@noisysocks noisysocks merged commit fcb90f8 into master Feb 18, 2019
@noisysocks noisysocks deleted the try/image-format-resize branch February 18, 2019 00:51
@ellatrix
Copy link
Member Author

Thanks @noisysocks! Cc @jasmussen for the design. I think the pop up should look closer to the one we have for links.

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Basic functionality

* Only update width on submit

* Fix styles

* Push a little polish

* Select objects when clicked

* Fix getDerivedStateFromProps

* Reselect image after update

* Move PositionedAtSelection

* Export PositionAtSelection in same way we export other @wordpress/components
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Basic functionality

* Only update width on submit

* Fix styles

* Push a little polish

* Select objects when clicked

* Fix getDerivedStateFromProps

* Reselect image after update

* Move PositionedAtSelection

* Export PositionAtSelection in same way we export other @wordpress/components
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Format library /packages/format-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants