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

Fixed issue with re-focusing PostTitle #14969

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Apr 12, 2019

Fixes: wordpress-mobile/gutenberg-mobile#865

Corresponding gutenberg-mobile PR : #14969

@marecar3 marecar3 added the Mobile Web Viewport sizes for mobile and tablet devices label Apr 12, 2019
@marecar3 marecar3 self-assigned this Apr 12, 2019
Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -681,7 +681,7 @@ const RichTextContainer = compose( [
return {
clientId: context.clientId,
isSelected: context.isSelected,
onFocus: context.onFocus,
onFocus: context.onFocus ? context.onFocus : ownProps.onFocus,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the ownProps.onFocus take precedence maybe? That's the callback set by the parent component, right? If so, my guess would be that it should override the internal callback?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure of the usage of context at that point to be honest. I didn't have time to investigate it properly last Friday.

It seems to me that this return will be executed if isSelected is undefined. And that will happen (probably) only on elements that are not blocks (like the PostTitle?). If that the case, maybe it's good to return ownProps directly?

I'm not sure of any of this, but this proposed solution fixes the bug on the release branch. Good thing is that there is time to think on a better solution. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure of the usage of context at that point to be honest. I didn't have time to investigate it properly last Friday.

👍 cool, no problem. Let's keep working on this one then to get to the bottom of it. @marecar3, I think Eduardo will not be available the next couple of days so please, feel free to dig into this one some more. Thanks!

@etoledom
Copy link
Contributor

Closing in favor of #15069

@etoledom etoledom closed this Apr 19, 2019
@marecar3 marecar3 deleted the rnmobile/hotfix_865-Post-Title-cant-be-re-focused-after-focus-has-changed branch April 26, 2019 03:12
@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
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.

5 participants