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 paragraph block writing flow tests #821

Merged
merged 69 commits into from
May 21, 2019
Merged

Conversation

JavonDavis
Copy link
Contributor

@JavonDavis JavonDavis commented Apr 4, 2019

Part of #745

This PR adds the following tests covering writing flows with the Paragraph block

  • Create a long post with a multiple Paragraph blocks
  • Have a paragraph block with a word in it. Place the cursor in the middle of the work and tap "Enter". Verify that the block split in two paragraph blocks, each with the respective half word.
  • Have two paragraph blocks with some content in each. Place the cursor at the beginning of the second and tap "Backspace". Verify that the blocks merged.

Additional notes

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 5, 2019

Warnings
⚠️ PR is not assigned to a milestone.
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@JavonDavis JavonDavis self-assigned this Apr 5, 2019
@JavonDavis JavonDavis added the Testing Anything related to automated tests label Apr 5, 2019
@JavonDavis JavonDavis marked this pull request as ready for review April 8, 2019 16:32
@JavonDavis JavonDavis requested review from Tug and hypest April 9, 2019 00:00
@JavonDavis JavonDavis requested review from etoledom and Tug May 16, 2019 22:26
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Thank you for the update @JavonDavis !
I added a couple of comments about accessibility labels, but it's looking quite good already.

Please let me know what do you think about the requested changes.


const removeButtonTitle = sprintf(
/* translators: accessibility text. %1: current block position (number). */
__( 'Remove row %1$s' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better if it says Remove block at row %s, since the action is to remove a block.
Note that when there's just one variable, %1$s is not necessary, just %s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we had previously discussed taking the word block out of the text since the block name would have already been said in VoiceOver once the block is selected, WDYT?

order,
order - 1 ) : sprintf(
/* translators: accessibility text. %d: block position (number) */
__( 'Move up from row %1$s' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we can say here just Move up, for this case where the block can't be moved up.
The system will add a dimmed | disabled to let the user know that the action is disabled. And the user will knowledge the intent of the button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think both messages are ok but, I agree removing the row number in this case this lines up better states the intent of the button when disabled and can be better for accessibility. As it relates to the tests this shouldn't have any impact so 👍

order,
order + 1 ) : sprintf(
/* translators: accessibility text. %d: block position (number) */
__( 'Move down from row %1$s' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change Move up, it would be nice to change this one too. 👍

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you @JavonDavis for all your hard work 🎉

@JavonDavis JavonDavis merged commit 2e18402 into develop May 21, 2019
@hypest hypest deleted the add/paragraph-flow-tests branch May 22, 2019 05:04
hypest added a commit that referenced this pull request May 22, 2019
…ge-not-loaded

Re-load the ReactVideoPackage that got removed by #821
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Anything related to automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants