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

Blur post title any time a new block is added #1231

Merged
merged 2 commits into from
Jul 19, 2019

Conversation

mchowning
Copy link
Contributor

Related gutenberg PR

Description

Fixes #932 , which was the problem that any time the post title was selected and a new image block was added, the post title would retain focus (i.e., the cursor). In fact, this issue occurred anytime a non-text block is added from the post-title. In particular, adding image, video, more, seperator, or page break blocks all leave focus with the post's title if it had focus immediately before the new block was added.

Before After
title-focus_before mp4 title-focus_after mp4

Implementation and Alternative Approaches

This PR insures the post-title is not selected when a new block is created by updating the post title to not be selected any time there is a selected block in the redux store (the post's title itself is not considered a selected block).

An alternative approach would have been to have the visual-editor give the block-list a callback that would set the visual-editor's isPostTitleSelected prop to false. The block-list could then execute that callback any time that a new block was added via its onBlockTypeSelected method. I did not use this approach because the redux store already knows whether a block is selected, and I wanted to avoid communicating that same information in a second way.

Another approach would be to move the post title's selected state itself into the redux store. This would have the advantage of simplifying the passing of that state information. Currently, the visual-editor holds that state so it can pass it down to both the block-list and post-title components. In addition, we are passing a ref to the post-title all the way up to the Editor component (post-title -> visual-editor -> layout -> edit-post) so that the post title can be selected from the native side. I did not go with this approach because both of the reasons we need to share/update the post-title's state are mobile specific, so this would cause the store for mobile to be different from web.

Would appreciate any other opinions on how best to approach this.

Testing

Fixes #932

  1. Open a post
  2. Select the post's title
  3. Add any "non-text" block (image, video, more, separator, or page break)
  4. Observe that there is no longer an active cursor on the post's title.

Regression check

  1. Open WPAndroid/WPiOS
  2. Create a new (empty) post
  3. Observe that the post's title is selected with an active cursor

Update release notes

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@mchowning
Copy link
Contributor Author

mchowning commented Jul 17, 2019

I'm labeling this "not ready for review" while I'm looking into the e2e tests it looks like I broke. 😔

@marecar3
Copy link
Contributor

Hey, @mchowning I saw a similar error on master -> develop PR, which I have resolved with just re-kicking the CI.

@mchowning
Copy link
Contributor Author

I saw a similar error on master -> develop PR, which I have resolved with just re-kicking the CI.

Thanks @marecar3 ! Ironically, it was seeing that your PR passed the build that made me think I must have broken it with my PR. 😆

Looks like I'm green now that the fixes were merged though. 🎉

@mchowning mchowning force-pushed the issue/932_blur-post-title branch 2 times, most recently from c0b9e88 to cb8f1c6 Compare July 19, 2019 10:19
@JavonDavis
Copy link
Contributor

@mchowning @marecar3 Yeah it seems when there's too many jobs queued up to be ran on the 5 available concurrent emulators if a job stays in the queue too long it'll hit the timeout waiting for the run to start, currently thinking of ways to avoid this but in scenarios like that where the test hasn't even kicked off due to a timeout, re-running the job should do the trick(granted there aren't other jobs waiting 😅 )

@marecar3
Copy link
Contributor

Hey @JavonDavis, thanks for elaboration on this one :)

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Nice work @mchowning!
LGTM!

@mchowning mchowning force-pushed the issue/932_blur-post-title branch from cb8f1c6 to 66dd13d Compare July 19, 2019 21:12
@mchowning mchowning merged commit 5e100b0 into develop Jul 19, 2019
@mchowning mchowning deleted the issue/932_blur-post-title branch July 19, 2019 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post Title stay focused after new image block is added
3 participants