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] Refactor BlockToolbar follow-up PR #16906

Merged
merged 4 commits into from Aug 5, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Aug 5, 2019

Description

This PR fixes some minor issues with #16677

How has this been tested?

Tested with wordpress-mobile/gutenberg-mobile#1247

Types of changes

UX changes:

  • Hide keyboard when switching mode
  • Do not force hide keyboard on android, as unselecting a block already handles it

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.

@Tug Tug added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Aug 5, 2019
@Tug Tug added this to the Future milestone Aug 5, 2019
@Tug Tug requested a review from mchowning August 5, 2019 16:22
@Tug Tug self-assigned this Aug 5, 2019
@Tug Tug requested a review from hypest August 5, 2019 16:24
@@ -158,6 +160,7 @@ export default compose( [
} ),
withDispatch( ( dispatch ) => {
const {
clearSelectedBlock,
Copy link
Contributor

@mchowning mchowning Aug 5, 2019

Choose a reason for hiding this comment

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

It looks like clearSelectedBlock from core/editor just calls through to core/block-editor with a deprecation warning that shows up as a Yellow Box warning. Sounds like we should just get this directly from core/block-editor instead of via core/editor.

Screen Shot 2019-08-05 at 12 55 51 PM

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, updated 👍

Copy link
Contributor

@mchowning mchowning Aug 5, 2019

Choose a reason for hiding this comment

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

I think we also need to update header-toolbar: https://github.com/WordPress/gutenberg/pull/16906/files#diff-dbd7e14b9675d5446422c008453093bbR91. Sorry I didn't realize that before.

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@Tug Tug merged commit 54323e9 into master Aug 5, 2019
@Tug Tug deleted the refactor/block-toolbar-fu-fix branch August 5, 2019 18:23
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.3 Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Keyboard is already hidden with clearSelectedBlock

* Hide Keyboard when switching mode

* require clearSelectedBlock from core/block-editor instead of core/editor

* require clearSelectedBlock from core/block-editor instead of core/editor
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Keyboard is already hidden with clearSelectedBlock

* Hide Keyboard when switching mode

* require clearSelectedBlock from core/block-editor instead of core/editor

* require clearSelectedBlock from core/block-editor instead of core/editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants