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] Blur post title any time another block is selected #16642

Merged
merged 2 commits into from Jul 19, 2019

Conversation

@mchowning
Copy link
Contributor

commented Jul 17, 2019

Description

This is a bug-fix that updates the post-title to not be selected if another block is selected. Additional description and testing steps are included in the related gutenberg-mobile PR.

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.
this.titleViewRef.focus();
this.props.onSelect();
}
this.props.onSelect();

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 17, 2019

Author Contributor

Because the selected state is being passed down as the RichText component's __unstableIsSelected prop, we no longer need to keep a ref and manually call focus. When the __unstableIsSelected prop is updated to true, the RichText component takes care of focusing itself.

@mchowning mchowning referenced this pull request Jul 17, 2019
1 of 1 task complete

@mchowning mchowning requested review from Tug, marecar3 and etoledom Jul 17, 2019

@@ -26,6 +26,13 @@ class VisualEditor extends Component {
};
}

static getDerivedStateFromProps( props, state ) {
if ( props.isAnyBlockSelected && state.isPostTitleSelected ) {

This comment has been minimized.

Copy link
@hypest

hypest Jul 18, 2019

Contributor

I wonder, can we remove the && state.isPostTitleSelected? Like, as the PR description says anyway, the Post title and other blocks selection is mutually exclusive, right? If any block is selected, isPostTitleSelected needs to be false regardless of its previous state, no? I might be missing some context here though so, please feel free to elaborate. Thanks!

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 18, 2019

Author Contributor

Good idea, I've made this change.

I believe I had that check just to avoid any possible risk of unnecessary re-renders, but looking at it again I think you're right and it's not needed. I can't find any documentation saying that kind of check is necessary, and I did some testing to confirm that removing the && state.isPostTitleSelected part of the check does not cause any additional renders. Thanks @hypest !

@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

I notice that some GitHub actions are still failing for reasons unclear, and chatting over Slack it seems that @talldan was recently able to fix the same problem on a PR by rebasing (on latest master I guess) and force-pushing. Can you try that @mchowning ? Thanks!

@mchowning mchowning force-pushed the rnmobile/blur-post-title branch from 488a3fb to 3da800f Jul 19, 2019

@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

it seems that @talldan was recently able to fix the same problem on a PR by rebasing (on latest master I guess) and force-pushing. Can you try that @mchowning ?

It worked! Thanks @hypest!

@marecar3
Copy link
Contributor

left a comment

Nice work @mchowning!
LGTM!

@mchowning mchowning merged commit b1b9c11 into master Jul 19, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@mchowning mchowning deleted the rnmobile/blur-post-title branch Jul 19, 2019

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

[RNMobile] Blur post title any time another block is selected (WordPr…
…ess#16642)

* Unselect post title any time another block is selected
* Remove unnecessary state check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.