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

Block Library: Add caption alignment to the image block. #20015

Closed
wants to merge 1 commit into from

Conversation

epiqueras
Copy link
Contributor

Closes #19975

Description

This PR adds text alignment to the Image block's caption.

How has this been tested?

It was verified that setting the text alignment on the Image block's caption works as expected.

Screenshots

Editor

Screen Shot 2020-02-03 at 4 03 42 PM

Front End

Screen Shot 2020-02-03 at 4 03 55 PM

Types of Changes

New Feature: You can now align the captions in Image blocks.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@epiqueras epiqueras added [Type] Enhancement A suggestion for improvement. [Block] Image Affects the Image Block labels Feb 3, 2020
@epiqueras epiqueras self-assigned this Feb 3, 2020
@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 4, 2020

It is awesome that caption alignment is being added!

I thought that caption alignment would be a part of the bottom caption toolbar?
As having it in the top image toolbar might be a bit confusing. The caption toolbar contain various formatting that has to do with the caption, and it seems alignment also would belong in it.

Screen Shot 2020-02-04 at 01 35 28

@epiqueras
Copy link
Contributor Author

epiqueras commented Feb 4, 2020 via email

@mapk
Copy link
Contributor

mapk commented Feb 4, 2020

Glad to see this getting in too! I agree with @paaljoachim that we should add this to the Captions toolbar though. Using it on the block toolbar alludes that everything within the block would be aligned a particular way.

If it’s at the bottom you won’t see it until you select the caption.

This is okay. Once you're typing a caption, it appears.

@paaljoachim
Copy link
Contributor

    • preadded caption -
      If one has added an caption earlier with the image in the Media Library then it will show up below the image. If the user does not click into the caption the user will not notice the caption toolbar and the various formatting one can do with a caption.
    • Has not preadded a caption. -
      If the user has not earlier added a caption then they will click into the caption area and notice the caption toolbar.

If a user wants to do something that affects the caption it seems natural based on clicking into any block to affect it that they will also click into the caption area and then notice the caption toolbar.

@epiqueras
Copy link
Contributor Author

epiqueras commented Feb 4, 2020 via email

@phpbits
Copy link
Contributor

phpbits commented Feb 4, 2020

@epiqueras Would it be better to add separate classnames? I'm using caption-align-left caption-align-center and caption-align-right on EditorsKit plugin. Thanks!

@epiqueras
Copy link
Contributor Author

No, we need to follow the standard to support theming.

@phpbits
Copy link
Contributor

phpbits commented Feb 4, 2020

@epiqueras I would love to check the theming standard, do you have the link for the classnames standard? Using has-text-align- works on this PR but when you add this caption alignment globally on other blocks that has caption you'll have a conflict. For example, the Table Block has caption as well and if you add has-text-align-center class on it all the text in the table will be aligned center. I hope this makes sense.

I know it might sound like I just want backward compatibility with EditorsKit because I'm using caption-align-{align} but I hope you'll reconsider.

Thanks!

@epiqueras
Copy link
Contributor Author

For example, the Table Block has caption as well and if you add has-text-align-center class on it all the text in the table will be aligned center. I hope this makes sense.

The class is applied to the caption, not the entire block.

I would love to check the theming standard, do you have the link for the classnames standard?

@jorgefilipecosta Do we have documentation for this somewhere?

@phpbits
Copy link
Contributor

phpbits commented Feb 4, 2020

The class is applied to the caption, not the entire block.

Oh! Sorry, forgot about that. I think it should be okay then. Thanks!

@mapk
Copy link
Contributor

mapk commented Feb 5, 2020

There was a good discussion about this in today's core-editor chat.

As a user, when realigning on the block-level, I expect everything in the block to realign. For example, take a look at the Paragraph block with an inline image. Everything realigns even though the icon is "text" alignment.

Screen Shot 2020-02-05 at 6 42 04 AM

Adding this to the block-level toolbar gives the perception that everything in the Image block, including the image, will realign. For this reason, it's important to add this to the caption toolbar because this alignment ONLY applies to the caption.

While this may be technically different from the other caption controls, it's no different to the user. If alignment only changes the caption, but is located in the block-level toolbar, why isn't the Bold, Italic, etc. options? These all apply to the same thing from a user's perspective. Having them separate will cause some confusion.

@shaunandrews
Copy link
Contributor

it's important to add this to the caption toolbar because this alignment ONLY applies to the caption.

Agreed here. If I think of the caption as a "Caption" child block of the Image block, then this makes even more sense.

@epiqueras
Copy link
Contributor Author

I don't think it's any more confusing than mixing inline rich text formats with a block-level caption alignment option.

Maybe "Change caption alignment." is clearer?

I suggest we ship this until RichText supports something like what you are suggesting. Otherwise, I'll close this for now.

@mapk
Copy link
Contributor

mapk commented Feb 5, 2020

I believe the disconnect between the caption controls is too confusing. I'm in favor of holding off on this until we can get the alignment controls in the Caption toolbar next to the other caption-specific controls.

@epiqueras I really appreciate your work here! Thank you!

@epiqueras epiqueras closed this Feb 5, 2020
@paaljoachim
Copy link
Contributor

I went ahead and added some more information to the issue: #19975 (comment)

@aristath aristath deleted the add/image-block-caption-alignment branch November 10, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing text-align for image caption
5 participants