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

[Mobile] Add unit tests for willTrimSpaces function in RichText #15552

Merged
merged 9 commits into from
May 22, 2019
Merged

[Mobile] Add unit tests for willTrimSpaces function in RichText #15552

merged 9 commits into from
May 22, 2019

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented May 10, 2019 •
edited
Loading

Description

This PR adds unit tests to specify the behavior for the willTrimSpaces function in RichText (https://github.com/WordPress/gutenberg/pull/15021/files#diff-4828a21853e899e5a36faecfa96d83e8R631). Unit tests should help in addressing some edge cases, such as the one described here: wordpress-mobile/gutenberg-mobile#903 (comment). The changes will also fix the regression of this issue #14957 described here: wordpress-mobile/gutenberg-mobile#903.

How has this been tested?

The tests are run from the gutenberg-mobile repository with yarn test, or specifically: yarn test gutenberg/packages/block-editor/src/components/rich-text/test/index.native.js.

Types of changes

Bug (regression) fix, and tests added.

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.

@mkevins mkevins added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Mobile Web Viewport sizes for mobile and tablet devices labels May 10, 2019
@mkevins mkevins requested review from hypest and marecar3 May 10, 2019 06:33
This was referenced May 17, 2019
Merged
Merged
@mkevins
Revert "Fix willTripSpaces to report false for no outer spaces on sty…
2c8e9af 
…led text."

This reverts commit 0731689.

Upstream changes make this implementation no longer necessary.
@mkevins mkevins changed the base branch from master to rnmobile/move-mobile-tests-to-gutenberg-repo May 20, 2019 09:35
@mkevins mkevins requested review from etoledom and removed request for hypest and marecar3 May 20, 2019 09:37
@mkevins
Merge remote-tracking branch 'origin/rnmobile/move-mobile-tests-to-gu…
3fe59f2 
…tenberg-repo' into try/add-unit-test-willTrimSpaces
@mkevins mkevins force-pushed the try/add-unit-test-willTrimSpaces branch from f507d2b to 3fe59f2 Compare May 20, 2019 09:47
etoledom
etoledom reviewed May 20, 2019
View reviewed changes
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.

Just one question:
We added the .native.js ignore pattern, but this test has extension .native.js, and it's meant to be run by stand-alone gutenberg repo.

Wouldn't this config actually ignore this test?

mkevins
mkevins commented May 21, 2019
View reviewed changes
packages/block-editor/src/components/rich-text/test/index.native.js Outdated
/**
* Internal dependencies
*/
import { RichText } from '../index.native';
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 imported from ../index.native because I was originally using this within the gutenberg test pipeline (without altering the jest config). I'm unsure whether this will be necessary if we plan to introduce a separate mobile-specific jest configuration which resolves all *.native.js files to their *.js key-names in the dependency map. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Running with gutenberg-mobile tooling, importing ../index will be enough 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @etoledom ! Fixed ✔️

@mkevins mkevins marked this pull request as ready for review May 21, 2019 14:08
@mkevins mkevins mentioned this pull request May 21, 2019
Merged
@mkevins
Copy link
Contributor Author

mkevins commented May 21, 2019

Just one question:
We added the .native.js ignore pattern, but this test has extension .native.js, and it's meant to be run by stand-alone gutenberg repo.

Wouldn't this config actually ignore this test?

Indeed. When I set out to add these unit tests, I envisioned running them side by side as part of the normal pipeline in the gutenberg repository. With mobile configuration we are moving towards in the interim, this should be ok, as tests like these will now be picked up by the mobile config.

etoledom
etoledom approved these changes May 21, 2019
View reviewed changes
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 @mkevins - Looks great! 🎉

@mkevins mkevins merged commit 35c5fa9 into rnmobile/move-mobile-tests-to-gutenberg-repo May 22, 2019
@mkevins mkevins deleted the try/add-unit-test-willTrimSpaces branch May 22, 2019 00:44
etoledom added a commit that referenced this pull request Jun 4, 2019
@etoledom
[RNMobile] Moving native tests to gutenberg repo (#15589)
1a293b3 
* Moving paragraph native test to gutenberg repo

* Rename native tests folder from __native_tests__ to test
To be able to use the same structure already in place.

* Ignore .native.js files on unit tests

* Move native code block test to gutenberg repo.

* Moved native link modal tests to gutenberg repo

* Moved mobile media progress tests to gutenberg repo.

* Moved mobile media upload tests to gutenberg repo

* Renamed mobile paragraph block tests to be the same as the file it tests

* [Mobile] Add unit tests for willTrimSpaces function in RichText (#15552)

* Add unit test for willTrimSpaces.

* Fix willTripSpaces to report false for no outer spaces on styled text.

* Ignore gutenberg-mobile unit tests in gutenberg test pipeline

* Revert "Fix willTripSpaces to report false for no outer spaces on styled text."

This reverts commit 0731689.

Upstream changes make this implementation no longer necessary.

* Fix lint issues

* Import RichText without native extension

* Remove stubs from rich-text __mocks__

* Update native paragraph test

* Update mobile link tests
@gziolo gziolo added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) and removed Mobile Web Viewport sizes for mobile and tablet devices labels Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etoledom etoledom etoledom approved these changes

Assignees
No one assigned
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable 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.
3 participants
@mkevins @etoledom @gziolo