[RNmobile] Scroll to new-block indicator #31144
Closed
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
|
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
Works but the placement of the code that scrolls is not proper. Should not be in the render function.
On iOS we use a ScrollView so, utilizing the scrollTo to specific offset. Computing the offset on the spot as the ScrollView doesn't keep an internal cache of item offsets.
scrollToOffset doesn't require it.
isRootList coming from the props is still true, but the insertion point can have a different (non empty) root when insider innerblocks.
This way, there's always enough room for the insertion point and the inserter to be visible when adding a new block at the end of the content.
hypest
force-pushed
the
rnmobile/1672-scroll-to-new-block-indicator
branch
from
6e57b3f
to
7ab5f03
Compare
hypest
force-pushed
the
rnmobile/1672-scroll-to-new-block-indicator
branch
from
67f14b2
to
c2856b5
Compare
2 tasks
dcalhoun
reviewed
Jun 17, 2021
There was a problem hiding this comment.
The code/approach is looking good to me. However, I did notice that the rendering has a few issues from my testing.
- On iOS, the toolbar attached to the top of the keyboard remains visible and floating detached from the keyboard for a second or two when the keyboard is dismissed (see image and video). This may be a preexisting issue. It may also be a performance issue that only occurs within a non-production build.
- On Android, the scrolling is not consistently set to correct position to display the insert point, particularly for the last block (see video). I found it often jumped around and resulted in the insert point behind the keyboard.
- On both iOS and Android, the insert point gets "stuck" beneath the post title after the first time you focus the title field. I believe this may be a preexisting issue unrelated to this work, but I couldn't find a relevant GitHub Issue (see videos).
- On both iOS and Android, the lack of animation of scrolling makes the experience a bit jarring in my opinion. A future enhancement opportunity might be to explore animating the scroll to reduce the jarring nature of the jump.
Do you experience any of these? Do you think some of these are specific to running against a Metro server? I have not tested this work integrated into WPiOS and WPAndroid. I could do that next, if you'd like.
iOS Detached Toolbar Image
iOS Video
RPReplay_Final1623947828.MP4
Android Video
add.block.indicator.mp4
| <View style={ styles.blockListFooter } /> | ||
| <View | ||
| style={ [ | ||
| styles.blockListFooter, |
There was a problem hiding this comment.
It would be ideal to only apply this offset when the inserter is opened and animate it to avoid a jump, but I would imagine that might be difficult to accomplish. I wanted to provide the thought nonetheless. Maybe an opportunity for future enhancement.
|
Thanks for the review @dcalhoun !
Hmm, watching the video, I think the toolbar stays there hanging in mid-air but it's behind the block picker, right? I mean, I don't see it staying hanging and visible to the user for 2 seconds 🤔. Do I miss something perhaps? FWIW, this ticket might be relevant. (Will address the rest of your points in follow up comment) |
You are correct. My word choice in my original description was poor. The toolbar is only visible detached for a second or less — a single frame of the video. It is also likely related to performance and could be unnoticeable when running a production build. I may look to produce a production build on a device to see how this feature performs as well. |
Aha, yeah, there's some issue there. While debugging, it looks like the editor settings memo here doesn't get updated when the title selection state changes (I've verifying in the debugger that title's selection state does indeed change). I will debug some more. Edit: created a ticket, a high priority one at that, since the new block is inserted at the wrong position. |
hypest
added
the
Mobile App - i.e. Android or iOS
|
Closing as stale and out-of-date. |
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
Addresses wordpress-mobile/gutenberg-mobile#1672
Adds automatic scrolling to where the block inserter is on native mobile, when adding a new block (Block Picker active). Not covering the case of adding a new block inside a group/innerblock block.
Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#3414
TODO
TODOs I didn't get to but could be(?) worked on in separate PRs
How has this been tested?
Add right under the title
Content scrolls up
Scroll up when at the end of the content
scroll_new-block_indicator_from_end.mp4
Types of changes
-> New feature (non-breaking change which adds functionality)
Changes
shouldFlatListPreventAutomaticScrollas we always scroll now. Tracked down the original PR that introduced it and verified that the issues that PR was fixing is not happening.getFirstBlockVisible()UI tests helper function to sniff out the innermost component with the desired accessibility Id as the parents would also have the same Id and picking the first in the array returned a wrong component. And then the now enlarged block appender in the list footer would get triggered on iOS when clicking on it the returned component. Here's the fix commit.Checklist:
*.native.jsfiles for terms that need renaming or removal).