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

[Android Aztec] - Fixes refresh issues on merging blocks #212

Merged
merged 4 commits into from
Nov 6, 2018
Merged

[Android Aztec] - Fixes refresh issues on merging blocks #212

merged 4 commits into from
Nov 6, 2018

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Oct 30, 2018 •
edited
Loading

Description:

Fixes refresh issues in Android when merging blocks.

Moves #167 forward.

Related PRs:

Aztec RN Wrapper PR: wordpress-mobile/react-native-aztec#67 [Merged]

Testing:

  1. Make sure you have 2 blocks of the same type (paragraph x 2 or heading x2).
  2. Place the caret at the beginning of the bottom block.
  3. Press backspace.
  4. Make sure the resulting block is as expected and refreshed properly.

@daniloercoli
Update React Native Aztec hash
312f253
@daniloercoli
Merge branch 'master' of https://github.com/wordpress-mobile/gutenber…
4553a39 
…g-mobile into issue/167-merge-refresh-android

* 'master' of https://github.com/wordpress-mobile/gutenberg-mobile: (35 commits)
  Point to latest from RecyclerViewList
  Point to latest react-native-aztec
  Adding yarn.lock field for `turbo-combine-reducers`.
  Added turbo-combine-reducers dependency to fix failed builds
  Update subrepo ref to point to master again
  Update subrepo ref
  No need for change of default Jest platform
  Update subrepo ref
  Update subrepo ref to point to master again
  Update subrepo ref
  Update subrepo ref
  Update subrepo ref
  Update subrepo ref
  Add symlink in symlinked-packages-in-parent for notices
  Add symlink for notices package to fix the failing CI tests
  Update subrepo by merging from master
  Add support for nextpage block
  Fix lint warnings
  Mocking `react-native-gutenberg-bridge` to make iOS tests pass. From: jestjs/jest#2208 (comment)
  rn-gutenberg-bridge: Adding dumb iOS implementation to avoid errors.
  ...
@daniloercoli daniloercoli requested a review from mzorz November 1, 2018 18:06
@hypest
Copy link
Contributor

hypest commented Nov 6, 2018

Hmm, doesn't seem to work for me when following the steps @daniloercoli . The second block completely disappears. I think it did work only once though thought my tests.

@daniloercoli
Copy link
Contributor Author

The second block completely disappears

Not quite sure I've got the problem you're experiencing.
When merging 2 blocks the second block should disappear, and its content moved into the first (the one above).

@daniloercoli
Merge branch 'master' of https://github.com/wordpress-mobile/gutenber…
6952854 
…g-mobile into issue/167-merge-refresh-android

* 'master' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Use a temporary symlink to parent wpandroid
  Remove ref to turbo-combine-reducers Update GB hash
  Add `turbo-combine-reducers` dep and fix tests and build errors
  Update GB hash
  Remove the code that does block the merging of different types blocks
@hypest
Copy link
Contributor

hypest commented Nov 6, 2018

If it's any help, here's a video from my Pixel 2 XL where the second blocks just disappears on "backspace", along with its contents: https://cloudup.com/cSFnyuS3jpF

@daniloercoli
Copy link
Contributor Author

I think the problems with the caret that jumps to another block, and the other problem with the content of the 2nd block not being copied into the first, live already in master. I've just tested the iOS side, and happens to have the same problems. https://cloudup.com/ccxN4XcmrGp

Will open issues for those problems.

This PR just adds a delay on the onContentSizeChanged event on Android, but should not alter the content in any way.

@hypest
Copy link
Contributor

hypest commented Nov 6, 2018

I think I have more info. Try to make changes to both blocks before merging them @daniloercoli .

@hypest
Copy link
Contributor

hypest commented Nov 6, 2018 •
edited
Loading

Verified that the merging behavior is not working OK for me on master too @daniloercoli , so, it's not a problem of this PR. Thing is, I don't know how to then test the change in this PR. Any different steps or something different to pay attention to to verify these changes?

@daniloercoli
Copy link
Contributor Author

It should work without making changes to blocks.
Put the caret at the beginning of the 1st para block, and try to merge it with the Heading block above.

hypest
hypest approved these changes Nov 6, 2018
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Put the caret at the beginning of the 1st para block, and try to merge it with the Heading block above.

Thanks Danilo, tried those steps and it works better. Will give the 👍 here and continue with the follow up PR I see you opened already (#223).

@daniloercoli daniloercoli merged commit 4f78d2f into master Nov 6, 2018
@daniloercoli daniloercoli deleted the issue/167-merge-refresh-android branch November 6, 2018 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hypest hypest hypest approved these changes

@diegoreymendez diegoreymendez Awaiting requested review from diegoreymendez

@mzorz mzorz Awaiting requested review from mzorz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daniloercoli @hypest