Comment Template: Improve UX of inner block selection #38263
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
michalczaplinski
added
[Type] Bug
|
Size Change: +3.52 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Feb 1, 2022
There was a problem hiding this comment.
Nice work @michalczaplinski! This is testing pretty well 👍. I've left a comment where it might be possible to address that remaining flicker, and how we got it working in #36431. It wasn't the neatest approach, but seemed to work, in case it helps here.
For the other issue, where clicking on a block within another instance selects the parent template block instead, I think this is because we're not specifically clicking on a "real" block to select it. I made an attempt at trying to workaround that in #37519 by switching off pointer-events: none in the block preview, and then grabbing the clientId of the block from within the preview — however that wound up causing confusing issues with double-ups in the sidebar inspector controls. I'm not sure if that approach is really viable, but just thought I'd mention it in case it sparks any ideas! The comment here is where I got to late last year. I'm mostly focused on a couple of other tasks at the moment, but hope to eventually get back to trying to figure it out. Of course, if you beat me to it, I'm more than happy to review / test and then copy over to the Query Loop block, too 😀
| children | ||
| ) : ( | ||
| <BlockPreview | ||
| <MemoizedCommentTemplatePreview |
There was a problem hiding this comment.
This is currently only rendering the preview if it isn't the selected instance (which makes semantic sense). However, over in #36431, to remove the flicker when clicking between instances, I also needed to render the currently selected instance, but then used styling to set it to display: none. While it does wind up with an extra preview in the DOM, using styling to switch the visibility of the preview means that it doesn't need to be mounted from scratch, which appeared to be the expensive operation that caused the flicker.
|
Thanks so much for the comments @andrewserong ! 🙂 The approach from the #36431 solves the flicker issue just fine but I wonder if there is perhaps a better way to achieve this 🤔 I think that react shouldn't really be rendering anything in between rendering the inner blocks and the preview. Maybe this is an issue with About #37519: lt looks like there are multiple ways to achieve the same bug because we suffer from the same issue in the Comment Template (it doesn't show up consistently though) and I haven't implemented your suggestion yet 😅 . |
Status update
I think this should be good to merge now but I'd like more eyes on this still @gziolo @DAreRodz @c4rl0sbr4v0 🙏 |
No, I haven't had a chance yet to dig in further, since I've been focused on other areas recently. Glad that the short-term workaround works to remove the flicker, though! But yes, it would be good to clean this up further down the track if we can.
It's a very strange bug that one. I didn't manage to reproduce it in production for the Query Loop block, but only in #37519... I think it'll need a bit more digging, too. 🤔 |
cbravobernal
reviewed
Feb 8, 2022
There was a problem hiding this comment.
Code looks good!
The only issue that I found is that when I click on the first element, the reply is getting selected.
Screen.Recording.2022-02-08.at.11.14.45.mov
It is an improvement of we had before, so, if you prefer to land this PR and add an issue for that "weird" selection let me know and I will approve it asap!
|
Nice work. I agree that those issues should be addressed in as many steps as necessary. Every improvement landed is a win for users. |
|
Thanks for your comments guys! 2022-02-08_18-54-31.mp4
Indeed, I ll try to work out the remaining issues in the next PR! :) @c4rl0sbr4v0 |
gziolo
approved these changes
Feb 9, 2022
cbravobernal
approved these changes
Feb 9, 2022
13 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.
Screen.Recording.2022-01-26.at.21.19.06.mov
Fixes #37154 using the same technique as used in #36431.
Instead of using the
BlockPreviewcomponent, the Comment Template block is now using theuseBlockPreviewhook. The part that seems to improve the UX is NOT rendering the inserters after each of the blocks inside of the Comment Template.Also renamed
firstBlockprop tofirstCommentfor clarity.The UX seems a lot better now. Two outstanding issues that I ll tackle in following PRs:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).