Skip to content

Add text alignment prop for centering image captions#941

Merged
etoledom merged 7 commits intowordpress-mobile:feature/captionfrom
mchowning:feature/add_text_align_prop
May 10, 2019
Merged

Add text alignment prop for centering image captions#941
etoledom merged 7 commits intowordpress-mobile:feature/captionfrom
mchowning:feature/add_text_align_prop

Conversation

@mchowning
Copy link
Copy Markdown
Contributor

@mchowning mchowning commented Apr 30, 2019 •
edited
Loading

Description

This PR adds a textAlign property to the ReactAztecManager so the text in image captions can be centered. See the companion gutenberg PR for usage on the React Native side.

This addresses the loss of center alignment for image captions that was introduced when the image caption was switched to use a RichText component.

center_caption_resized

⚠️ Implementation of the textAlign prop still needs to be completed on iOS.

Test Cases

  1. An image caption's text should have center alignment when the image caption is not selected.
  2. An image caption's text should have center alignment when the image caption is selected.
  3. An image caption's text should have center alignment when the image caption is being edited.
  4. When an image with an empty caption is selected, the placeholder text "Write caption..." should appear centered.
  5. Image caption lines that are wider than the available space should wrap to a new line.
  6. Pressing the enter key while editing an image caption should add a new line to the image caption.
  7. It should be possible to add bold, italic, link, and strikethrough styling attributes to an image caption.

Issue Adding New Lines to Styled Text

One issue that exists on Android is that tapping enter to add a new line to a caption that has styles applied will frequently not move the cursor to the newly created line. I think that this is a broader issue with RichText components that span multiple lines (I'm looking at you list blocks), which will be resolved by @hypest's PR Fixing List Block Handling (see my comment on that PR discussing this issue). For that reason, I do not think this issue should block this PR.

@mchowning mchowning mentioned this pull request Apr 30, 2019
Merged
5 tasks
@mchowning
Copy link
Copy Markdown
Contributor Author

mchowning commented Apr 30, 2019 •
edited
Loading

Hey @etoledom ! 👋 Can you look into the iOS implementation for this when you have some time? I'm hoping we can take some inspiration from how Reat native's RCTBaseTextViewManager adds an NSTextAlignment textAlign property to update our RCTAztecViewManager to export a textAlign property to react. Of course, I can't actually test this out, so I'm sure it won't be that easy! 😄

@mchowning mchowning changed the title Add text-align prop for centering image captions Add textAlign prop for centering image captions Apr 30, 2019
@mchowning mchowning changed the title Add textAlign prop for centering image captions Add text alignment prop for centering image captions Apr 30, 2019
@mchowning mchowning force-pushed the feature/add_text_align_prop branch from 87549c9 to 0f23262 Compare April 30, 2019 02:34
@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented Apr 30, 2019

Thank you @mchowning ! No worries, I'll check the iOS part 👍

@etoledom etoledom added the [Type] Enhancement Improves a current area of the editor label Apr 30, 2019
@etoledom etoledom added this to the v1.4 milestone Apr 30, 2019
@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented May 1, 2019 •
edited
Loading

Added a commit that handles text alignment on iOS Aztec Wrapper.

IMG_1668

Note: New lines on iOS is not yet handled correctly. ref: WordPress/gutenberg#14883 (review)
Fixed here: #945

@SergioEstevao could you take a look at the iOS part?
And @hypest, @daniloercoli, @marecar3, @mkevins, could any of you review the Android side of this PR?
Thank you! 🙏

@etoledom etoledom modified the milestones: v1.4, v1.5 May 3, 2019
@mkevins
Copy link
Copy Markdown
Contributor

mkevins commented May 3, 2019

I have tested this via Android emulator (Pixel 2 on API 27). All 7 cases in the description function as expected. However, I observed an issue where the text after the caret is lost after pressing [Backspace] to merge the two lines, followed by pressing [Enter] to split the line back to two.

Steps to reproduce:

  1. Tap the image
  2. Tap the caption area
  3. Type "Test"
  4. Press [Enter] (observe that the caret moves to a new line)
  5. Type "Test"
  6. Tap to the left of the second line (observe that the caret moves to the beginning of the second line)
  7. Press [Backspace] (observe that the two lines are merged into one, with the text: "TestTest")
  8. Press [Enter]

Expected result:

The caption has 2 lines with the text: "Test" on each line.

Actual result:

The caption has 1 line with the text: "Test", and the 2nd line is empty.

Screencast:

@mchowning
Copy link
Copy Markdown
Contributor Author

mchowning commented May 3, 2019

Thanks for updating the feature branches with the latest from develop @etoledom ! I have merged those changes into both this PR and the gutenberg PR.

@mkevins: this should be ready for retest. In my testing, the issue you observed is fixed by the latest changes that were pulled in from develop.

⚠️ One note on my merge. There was a small conflict on the iOS side that I had to resolve manually. It seemed like a pretty straightforward conflict to resolve (famous last words), but since I cannot check iOS myself, I would like someone to double check that my merge did not break iOS. My manual conflict resolution is in a separate commit from the automatic merge. The conflict arose because a line that was removed on develop got a minor formatting change in this PR.

@mkevins
Copy link
Copy Markdown
Contributor

mkevins commented May 3, 2019

I tested the cases again, as well as the previously reported issue, and everything is now working as expected. 🎉

@etoledom etoledom requested a review from SergioEstevao May 3, 2019 16:31
SergioEstevao
SergioEstevao approved these changes May 7, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Tested it on iOS and it's working great.

@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented May 10, 2019

There is some issue with UI Tests on PRs from external contributors.
Merging since UITests are passing locally.

@etoledom etoledom merged commit c57f200 into wordpress-mobile:feature/caption May 10, 2019
@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented May 10, 2019 •
edited
Loading

Great job @mchowning ! 🎉

@etoledom etoledom mentioned this pull request May 10, 2019
Merged
@mchowning mchowning deleted the feature/add_text_align_prop branch May 25, 2019 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SergioEstevao SergioEstevao SergioEstevao approved these changes

Assignees

@mchowning mchowning

Labels

[Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

v1.5

Development

Successfully merging this pull request may close these issues.

4 participants

@mchowning @etoledom @mkevins @SergioEstevao