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.

Sign up for GitHub

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

Jump to bottom

Fix caption alignment on images. #18818

Merged
merged 1 commit into from
Nov 29, 2019
Merged

Fix caption alignment on images. #18818

merged 1 commit into from
Nov 29, 2019

Conversation

SergioEstevao
Copy link
Contributor

Description

Fix caption alignment in Native.
This sorts the problem in iOS
Please test this in Android!

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

@SergioEstevao SergioEstevao added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 29, 2019
pinarol
pinarol reviewed Nov 29, 2019
View reviewed changes
packages/block-editor/src/components/caption/index.native.js
@@ -29,7 +29,7 @@ const Caption = ( { accessible, accessibilityLabel, onBlur, onChange, onFocus, i
fontSize={ 14 }
underlineColorAndroid="transparent"
textAlign={ 'center' }
tagName={ '' }
tagName={ 'p' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I got really curious about this one. how's giving a tag name fixing the alignment? 🤔

and, is it ok to have both at the same time?

rootTagsToEliminate={ [ 'p' ] }

tagName={ 'p' }

Copy link
Contributor Author

@SergioEstevao SergioEstevao Nov 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't have time to do a full investigation, but main main guess, is that when you don't define a tag, the default you get is div, and the Aztec iOS for some reason is ignoring the text alignemnt settings or defaulting the div to left alignment.

The rootTagsToEliminate is just to give the HTML back clean of <p> know that we are using the tagName of p we should no longer need it, because the component already eliminates automatically the tag refered in tagName.

Copy link
Contributor

@pinarol pinarol Nov 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't verified but suspicious that this might have caused a regression like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't verified but suspicious that this might have caused a regression like this.

@cameronvoell has reached the same conclusion last night so, I guess that's a form of verification.

@pinarol pinarol mentioned this pull request Nov 29, 2019
Closed
@pinarol
Copy link
Contributor

pinarol commented Nov 29, 2019 •
edited
Loading

cc @mkevins we'll need to do the same fix for the captions of individual gallery tiles. Ideally we should refactor current Caption component and extract the UI part to make it available to use from gallery tiles as well, otherwise we'll need to duplicate fixes like this. I've opened a tech debt issue to track this.

@maxme maxme mentioned this pull request Nov 29, 2019
Closed
@maxme
Copy link
Contributor

maxme commented Nov 29, 2019

I tested on Android, everything seems fine (was also fine before the patch):

Screenshot_1575027197

@maxme
Copy link
Contributor

maxme commented Nov 29, 2019

Also tested on iOS, looks good:
Simulator Screen Shot - iPhone 11 - 2019-11-29 at 14 07 41

@maxme maxme merged commit 8ab1af9 into rnmobile/release-v1.18.0 Nov 29, 2019
@maxme maxme deleted the rnmobile/issue_fix_caption_align branch November 29, 2019 13:09
@pinarol pinarol mentioned this pull request Nov 29, 2019
Merged
5 tasks
@mchowning
Copy link
Contributor

mchowning commented Nov 29, 2019 •
edited
Loading

This change introduces a regression where pressing the Enter key in a caption no longer works to add a newline on Android. There was some discussion around this issue in a few of the comments on an earlier PR.

no-enter mp4

maxme added a commit that referenced this pull request Nov 29, 2019
@maxme
[RNMobile] Merge mobile release v1.18.0 back into master (#18804)
db956c4 
* [RNMobile] Enable Spacer block (#18605)

* Fix caption alignemnt on images. (#18818)

* Imagea Size: Force use original image URL when required slug does not exist
@SergioEstevao SergioEstevao changed the title Fix caption alignemnt on images. Fix caption alignment on images. Dec 2, 2019
@mchowning mchowning mentioned this pull request Dec 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hypest hypest hypest left review comments

@pinarol pinarol pinarol left review comments

@maxme maxme maxme approved these changes

@mchowning mchowning Awaiting requested review from mchowning

@etoledom etoledom Awaiting requested review from etoledom

Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SergioEstevao @pinarol @maxme @mchowning @hypest