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

[RNMobile] Fix for history stack that's is not empty on a fresh start of the editor #15055

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Apr 18, 2019

This PR does fix a problem where opening a post in GB mobile (without modify it) does result on having the Undo button already available/enabled in the toolbar.
It also fixes the other problem where removing a new block with Undo/Redo was not possible without fast clicking on the the Undo button.

In this PR i've just removed the call that refreshes the content into the store in onSelectChange. Not quite sure why we're sending back the content if it was not modified. In fact this.lastContent was set with the actual content to avoid a refresh of the component.

Testing steps in the GB Mobile PR here: wordpress-mobile/gutenberg-mobile#892

…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  Reset embed mocks on every request, stop request to instagram (#15046)
  Refactor core blocks to have save and transforms in their own files (part 2) (#14899)
  Fix pullquote import (#15036)
  Refactor core blocks to have save and transforms in their own files (part 4) (#14903)
  Refactor core blocks to have save and transforms in their own files (part 3) (#14902)
  Refactor core blocks to have deprecated extracted to their ownf files (p.1) (#14979)
  Test transform from media to embed blocks (#13997)
  If a more recent revision/autosave exists, store its state on editor setup (#7945)
  chore(release): publish
@mkevins
Copy link
Contributor

mkevins commented Apr 19, 2019

I tested this, and it does fix the issue, as described.

See this comment on the related gutenberg-mobile PR for additional details: wordpress-mobile/gutenberg-mobile#892 (comment).

@hypest
Copy link
Contributor

hypest commented Apr 19, 2019

👋 @daniloercoli and @mkevins , can you also check that the test cases in wordpress-mobile/gutenberg-mobile#885 are still performing OK? This change here could be relating to how the text/state is updated when Aztec trims the text so, let's make sure no regression happens there.

@mkevins
Copy link
Contributor

mkevins commented Apr 19, 2019

I have tested the cases described here: wordpress-mobile/gutenberg-mobile#885 (comment), and have found that cases A and B are still working as expected.

For case C, I found some inconsistent results when testing the original issue, which I've described here: wordpress-mobile/gutenberg-mobile#885 (comment). Testing the same case here, I observed that when the period was created after two spaces, the caret position was before the period, and the third space was inserted there.

Here is a screenshot of the behavior:

@daniloercoli
Copy link
Contributor Author

I'm going to merge this PR since case C. does already have inconsistent results with AOSP Keyboard in the original PR, and this PR doesn't change the way set the caret.
I might be wrong but we can tackle the issue on another PR.

@mkevins Would you mind to open an issue for your findings reported in the comment in the original PR?

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

This change fixes the undo issue for the case described. The issue described above (caret position before the period) can be addressed in a separate PR.

Edit:

I have found that the above described issue where the caret precedes the auto-punctuate period also occurs on paragraph and heading blocks after these changes.

@daniloercoli daniloercoli self-assigned this Apr 19, 2019
…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  Avoid running hasMetaBoxes on each subscribe (#15041)
  Avoid passing down isFirst and isLast props (#15043)
  Add "Roadmap" document with an overview of projects in consideration. (#14907)
  Testing: Update tests to use Escape press to activate block toolbar (#15063)
  Testing: Skip unreliable end-to-end tests (#15059)
@daniloercoli
Copy link
Contributor Author

I've updated this PR, and it should work as expected on lists, and para blocks, when double tapping the space bar on software keyboard. Ready for another round @mkevins

@mkevins mkevins self-requested a review April 22, 2019 13:11
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

The additional commit fixes the issue with the caret preceding the auto-punctuate period. Also, I tested the original issue that this fixes, and that is working well. 🎉 The undo button becoming "enabled" in the italicized block still happens, but as discussed, can be addressed in a separate PR. The undo of a new block once in that "state" is much smoother after this fix.

…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  'string' misspelled as 'srting' (#15118)
  [Mobile] Improving accessibility on Post title (#15106)
  Fix 13776: Format is already registered to handle class name (#15072)
  Add wpDataSelect WordPress end 2 end test util (#15052)
  Block Editor: move selection state from RichText to the store (#14640)
  chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073)
  Set ownProps.onFocus when context.onFocus is undefined. (#15069)
@daniloercoli daniloercoli merged commit 39f9fec into master Apr 23, 2019
hypest pushed a commit that referenced this pull request May 2, 2019
… of the editor (#15055)

* Do not reset the content in onSelectionChange

* Make sure there are changes made to the content before upgrading it upward

* Refactor: Introduce a local variable and DRY the code a bit

* Fix lint
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
@youknowriad youknowriad deleted the rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor branch May 27, 2020 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants