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

Add content alignment options to paragraph block #1489

Merged
merged 11 commits into from Nov 14, 2019

Conversation

@mchowning
Copy link
Contributor

mchowning commented Oct 23, 2019

Fixes #1266 . Further details available in the related gutenberg PR. Newer Gutenberg PR that targets the 1.17 release: WordPress/gutenberg#18433

Because this includes an update to the AztecEditor-iOS version, merging this also needs to be coordinated with merging that update into WPiOS: wordpress-mobile/WordPress-iOS#12929

Release Notes

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@mchowning mchowning added this to the 1.16 milestone Oct 23, 2019
@mchowning mchowning requested review from SergioEstevao and marecar3 Oct 23, 2019
@mchowning mchowning self-assigned this Oct 23, 2019
@SergioEstevao

This comment has been minimized.

Copy link
Contributor

SergioEstevao commented Oct 24, 2019

This is working pretty good but I found two edge cases were it's failing:

  1. If you have a paragraph block selected with empty text and the cursor on it, if you change the alignment the place holder label moves but the cursor stays on the old alignment and if you type the old alignment is kept

Simulator Screen Shot - iPhone 11 - 2019-10-24 at 11 44 30

  1. When selecting right alignment the placeholder text is cropped, see that the last dot of the ellipsis is not visible on the placeholder
    Simulator Screen Shot - iPhone 11 - 2019-10-24 at 11 42 58
@mchowning

This comment has been minimized.

Copy link
Contributor Author

mchowning commented Oct 24, 2019

Thanks for the review @SergioEstevao !

  1. If you have a paragraph block selected with empty text and the cursor on it, if you change the alignment the place holder label moves but the cursor stays on the old alignment and if you type the old alignment is kept

This doesn't seem to be happening for me. 🤔 I thought it might be a device/os-specific issue, but I've tried on a few different devices (physical iPhone Xr w/iOS 13, and simulators with iOS 12 & 11). What device and os are you using? Any unusual steps to reproduce?

  1. When selecting right alignment the placeholder text is cropped, see that the last dot of the ellipsis is not visible on the placeholder

Good catch! I didn't notice the missing third period, but I see it now that you've pointed it out. I'll look into it.

@mchowning

This comment has been minimized.

Copy link
Contributor Author

mchowning commented Oct 24, 2019

  1. When selecting right alignment the placeholder text is cropped, see that the last dot of the ellipsis is not visible on the placeholder

I've pushed a fix for this in the latest commit. Would be great if @etoledom could give that commit a look since it is tweaking some code he wrote for handling RTL.

For that reason, any testing of my changes should be sure to check that RTL is handled appropriately (I tested RTL on iOS with Run Options -> Application Language: Right-To-Left Pseudolanguage). In particular, keep an eye out that when an empty paragraph block is selected the cursor is appropriately positioned relative to the placeholder text.

@etoledom etoledom self-requested a review Oct 24, 2019
Copy link
Contributor

marecar3 left a comment

Nice work @mchowning!
LGTM!

@SergioEstevao

This comment has been minimized.

Copy link
Contributor

SergioEstevao commented Oct 28, 2019

@mchowning here is a video of the first bug I mentioned:

AlignBug

@mchowning

This comment has been minimized.

Copy link
Contributor Author

mchowning commented Oct 31, 2019

Thanks for the video @SergioEstevao !

Here are the steps to reproduce I'm seeing and some notes. I've spent some time digging into this and I am stumped so far. I'm not that familiar with Aztec-iOS, so please let me know if you have any suggestions on what I might want to investigate @SergioEstevao .

As an alternative, what would you think about opening a new issue for this and merging this as-is since it seems like this probably will not occur very often, and when it does occur it is not too difficult to recover alignment by just updating the alignment once text has been added?

Step # Step Info
1a (Alternative a) Load a post with a paragraph block that already has text The text can be either aligned or unaligned. The bug will not occur if you load a post with an empty paragraph block.
1b (Alternative b) Create a new block, add text to it, and then deselect and reselect that block. Focus must leave the block in question, if you remain focused on the block the whole time then the bug will not appear.
2 Delete all the text from the paragraph block The bug appears regardless of whether you change the alignment before deleting all the text in the block
3 Update the alignment of the empty block The block must be empty, if you delete all the text and add any text back, then the bug does not appear.
@hypest hypest modified the milestones: 1.16, 1.17 Nov 1, 2019
Copy link
Contributor

SergioEstevao left a comment

@mchowning ok lets approve this to move it forward.

Please open a ticket for the issue I found, in order for us to fix/track in the near future.

@SergioEstevao

This comment has been minimized.

Copy link
Contributor

SergioEstevao commented Nov 4, 2019

Remember to update the release notes to say this change is landing on release 1.17.0

@mchowning mchowning force-pushed the 1266_add_content_alignment_to_paragraphs branch 3 times, most recently from b0e9307 to fb5c799 Nov 5, 2019
@mchowning mchowning changed the base branch from develop to release/1.16.1 Nov 6, 2019
@mchowning mchowning changed the base branch from release/1.16.1 to develop Nov 6, 2019
@mchowning mchowning changed the base branch from develop to release/1.17 Nov 11, 2019
mchowning added 4 commits Oct 18, 2019
Insures the placeholder is properly positioned when "end" alignment
(i.e., right in LTR languages and left in RTL languages) is applied.
@mchowning mchowning force-pushed the 1266_add_content_alignment_to_paragraphs branch from 0825708 to 4cb36ac Nov 12, 2019
@hypest
hypest approved these changes Nov 14, 2019
Copy link
Contributor

hypest left a comment

LGTM!

Tested both on WPAndroid an WPiOS and is working nicely! 😍

@hypest hypest merged commit e1cf16c into release/1.17 Nov 14, 2019
5 checks passed
5 checks passed
ci/circleci: Check Correctness Your tests passed on CircleCI!
Details
ci/circleci: Test Android Your tests passed on CircleCI!
Details
ci/circleci: Test Android on Device Your tests passed on CircleCI!
Details
ci/circleci: Test iOS Your tests passed on CircleCI!
Details
ci/circleci: Test iOS on Device Your tests passed on CircleCI!
Details
@hypest hypest deleted the 1266_add_content_alignment_to_paragraphs branch Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.