Add native block inserter onboarding tooltip #32001
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Needs more accurate positioning, more styling, and better position prop support.
The Tooltip Slot is currently successfully invoking the callback, but the state is not triggering a re-render to hide the Tooltip.
Previously, _all_ tooltips registered a callback regardless if they were visible. This meant that whichever tooltip was last was the only tooltip registered. Often times this meant the currently visible tooltip would remain left with a stale visibility as its callback had been cleared.
Previously, the animation ran anytime the visible prop changed, even if it was the same value as the previous render.
The `visible` prop was added as the hover/focus trigger used for web is not relevant for touch devices. The ability to display the tooltip on initial render is required for the current use case of an onboarding experience.
Use a ref to avoid losing the timeout value. Rename the value to increase clarity of intent.
Include the phrase "ref" in the name to communicate the actual value.
Memoize object creation to avoid unnnecessary rerenders of the Provider's children.
Only display the editor onboarding tooltip for users who are first launching the editor.
Combing `select` calls to avoid multiple subscriptions that may(?) degrade performance.
This reverts commit e4f091d.
To avoid the need to render a wrapping `View` or forward a `ref` down the tree, the tooltip was moved further down the tree.
Rather than wrapping the block inserter button, we can leverage the Tooltip component that is included inside of Button. In order to do so, we must set `visible` whenever `showTooltip` is explicitly `true`.
`requestAnimationFrame` feels more appropriate than `setTimeout` given we are awaiting a render. This is required to await the render before calculating the layout values to position the tooltip.
We must reposition the tooltip when the keyboard position changes to avoid collisions.
Reposition or hide the tooltip depending on the keyboard visibility.
Enabling `showTooltip` within the dropdown menu was likely a copy from the web implementation. Previously the mobile implementation of tooltip was a no-op, rendering the `showTooltip` value moot. Now that tooltip renders UI, we must disable `showTooltip` on mobile, otherwise every dropdown menu button would display its tooltip on initial render.
Now that we rely upon `requestAnimationFrame` to ensure the render completes before calculation, there are times where the ref is no longer present once the callback completes. This change ensures we check within the `requestAnimationFrame` callback.
Add documentation and remove unnecessary hook dependency.
An error occurred during creating or cleaning up the Hook.
|
Size Change: 0 B Total Size: 1.86 MB ℹ️ View Unchanged
|
|
@jhnstn @kyleaparker this work is ready for an initial design and code review. Installable builds may be found on the WPiOS and WPAndroid PRs referenced in the PR description. Would you be willing to take a look please? Thanks! |
|
I tested with the WPAndroid installable build and 🎉 |
hypest
reviewed
May 21, 2021
hypest
reviewed
May 21, 2021
hypest
reviewed
May 21, 2021
|
Great work @dcalhoun, this works as expected for me! Are we planning to show this to all users? I'm wondering if we should be excluding people who are already familiar with adding content to the editor, but I'm not sure how we can detect this. |
@kyleaparker thanks for testing!
Sorry for the confusion. We are not showing this to all users. We plan to show this experience to a TBD percentage of users. However, the development/testing builds show this experience to every install, purely to make development/testing easier. That is why you see the tooltip each time you re-install the test build. Additionally, last week we merged wordpress-mobile/gutenberg-mobile#3437, which sets a "flag" whenever a user launches the Gutenberg block editor. So, if a user has launched the editor before, this flag would be set and they would not see the onboarding experience. If the flag is not set, they are considered a "new" user and would see the onboarding experience. Does that make sense? Happy to provide more details if it is helpful. |
The class member and prop were named identically, which may following the flow a bit difficult.
The `getSettings` value is already destructured earlier in the routine.
Renamed to avoid ambiguity.
jhnstn
reviewed
May 25, 2021
| const { | ||
| __experimentalShouldInsertAtTheTop: shouldInsertAtTheTop, | ||
| } = getSettings(); | ||
| } = getBlockEditorSettings(); |
jhnstn
reviewed
May 25, 2021
There was a problem hiding this comment.
I ran through the test steps on Android and iOS.
I found everything worked as expected in Android.
I notice a few things on iOS:
-
When opening a new post when the tooltip is active there is no focus on the title block. I expected the title block to be focused on when creating a new post.
-
When rotating to landscape the tooltip disappears but when rotating back to portrait the tooltip re appears. Other events do fully disable the tooltip as expected.
I wouldn't think the rotation disabling is a blocker but might want to confirm the title focus change is ok.
I'm using an iPhone 11 iOS, v14.4
AmandaRiu
approved these changes
May 25, 2021
There was a problem hiding this comment.
Hi @dcalhoun Tested WPiOS and WPAndroid - works great! Looked over the code and that looks really good as well, but I'm still learning and am not yet really familiar with the way SlotFills work and several other really interesting things in your code so I'd definitely feel better if there were a 2nd set of eyes on this one just to make sure I didn't miss anything. 👍
@jhnstn this focus issue appears unrelated and to occur in the latest App Store release (17.3), but it only occurs for the first launch after fresh installation. I have been unable to reproduce the issue outside of a fresh install. Are you able to confirm you experience the same? If so, I would think we should address that separately from this work. WDYT?
Agreed that the rotation issue is minor and not worth blocking this work. |
Confirmed. I just installed the latest from the App store and I'm seeing the same focus flow. |
jhnstn
approved these changes
May 26, 2021
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #30624. Adds a tooltip for the block inserter as a part of a larger editor onboarding experience.
A native implementation was added for the existing
Tooltipcomponent used on the web. This was done in an effort to (1) align the platforms and (2) avoid further duplicating tooltip logic. E.g. a duplicative tooltip component exists for the native Cover media focal point picker, and hopefully can one day be replaced by theTooltipimplementation in this PR.The
Buttoncomponent already renders the existing Tooltip component. Because of this, it was not necessary to wrap the block inserter dropdown menu button in a newTooltipelement. Instead, this PR leveragesButton'stitleandshowTooltipprops to display new "Tap to add content" tooltip required for the onboarding feature. One benefit of this approach is that we do not need to worry about forwarding a React ref through toButton.How has this been tested?
Automated tests were not added in this PR, as the feature is an experiment and may not remain. However, I do plan to create a follow up PR to add tests for the
Tooltipcomponent if this PR merges. The work in this PR was manually tested via the following flow:For Android, one can clear out the WordPress app data to re-test the flow. For iOS, the only option I am aware of is uninstalling and re-installing the app again. One thing to keep in mind is that the WPCom API places a limit on the number of times you may log in with a single account over a period of time. It will start a timeout if you log in too frequently. I'm uncertain of the number of attempts that triggers the timeout or how long the timeout lasts.
Screenshots
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).