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

Comment Template: Improve selection for the inner blocks #37154

Closed
gziolo opened this issue Dec 6, 2021 · 6 comments · Fixed by #38263
Closed

Comment Template: Improve selection for the inner blocks #37154

gziolo opened this issue Dec 6, 2021 · 6 comments · Fixed by #38263
Assignees
Labels
[Block] Comment Template Affects the Comment Template Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@gziolo
Copy link
Member

gziolo commented Dec 6, 2021

Description

Originally reported in #36065 (review):

Selecting blocks for nested comments sometimes changes the position on the screen in an unexpected way, you can observe that in the video I included:

Screen.Recording.2021-12-02.at.12.17.25.mov

Confirmed in #36065 (comment) by @michalczaplinski:

There is some issue with block focus/selection, related to what you mentioned in your last point. Sometimes, you have to click twice or three times in order to select the relevant block inside of the Template. And sometimes. even then, the an incorrect block gets selected initially.

It might be the same issue that @andrewserong is trying to resolve for Query Loop and Post Comment in #36431.

Step-by-step reproduction instructions

  1. Insert the Comments Query Loop block in the site template and make sure that it contains comments with replies.
  2. Try to select inner blocks in the Comment Template block for the nested (replies) comments.

Screenshots, screen recording, code snippet

Screen.Recording.2021-12-02.at.12.17.25.mov

Environment info

  • trunk in Gutenberg
  • WordPress 5.9 beta 1
  • Safari on macOS

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Block] Comment Template Affects the Comment Template Block labels Dec 6, 2021
@gziolo gziolo changed the title Comment Template: Comment Template: Improve selection for the inner blocks Dec 6, 2021
@andrewserong
Copy link
Contributor

It might be the same issue that @andrewserong is trying to resolve for Query Loop and Post Comment in #36431.

Thanks for the ping! That PR seeks to address part of the problem (preserve the wide layout alignment, remove flicker / unintentional scroll when switching the current block context). There's another problem to deal with, that I thought could be good for a follow-up, which is to see if it's possible in the click handler when switching block contexts, to also then set the currently selected block to the one that the user clicked on within the block preview. I'm not quite sure how feasible it is to do that neatly, so thought it'd make a good potential follow-up if that PR winds up landing. (Happy to explore within that PR if folks think it's better to try to deal with it all at once, of course!)

@gziolo
Copy link
Member Author

gziolo commented Dec 15, 2021

A quick update, I have just merged #36431 that radically improves the experience around the block selection for the Query Loop block.

There's another problem to deal with, that I thought could be good for a follow-up, which is to see if it's possible in the click handler when switching block contexts, to also then set the currently selected block to the one that the user clicked on within the block preview.

It's a very tricky issue we deal with here because we mostly show block previews but at the same time we have to support full interactivity for the currently selected block. Let us know when you have a prototype ready so we can stay in the loop.

@andrewserong
Copy link
Contributor

It's a very tricky issue we deal with here because we mostly show block previews but at the same time we have to support full interactivity for the currently selected block. Let us know when you have a prototype ready so we can stay in the loop.

Will do! Thanks for approving and merging in #36431 for me — now that we have that PR in, it should be easier to focus on this problem in isolation 🤞

@gziolo
Copy link
Member Author

gziolo commented Dec 21, 2021

The current approach we have has more drawbacks. Sometimes it renders the same controls multiple times:

Screenshot 2021-12-21 at 13 09 20

@andrewserong
Copy link
Contributor

I'm not sure if this helps, but I ran into a similar issue in my WIP PR in #37519 for the Post Template block when I removed pointer-events: none on the block preview — this meant that when clicking between the Query loop previews, the clicking on a block within the preview appeared to render the component and its InspectorControls fill, for the particular clientId (given that the clientId of the preview is the same for each instance of the loop). I think the pointer-events: none there (and earlier in the Disabled component) typically prevents this in the live preview, but I'm curious if there's a way to prevent the components within a block preview from ever rendering its slotfills / live behaviour.

Apologies if that doesn't quite make sense — I'm a little fuzzy on how those pieces interact. I haven't noticed the above issue in the Query Loop (not comments) block in testing on trunk, though.

@gziolo
Copy link
Member Author

gziolo commented Jan 31, 2022

@andrewserong, @michalczaplinski opened #38263 with initial changes necessary to mitigate the issues with inner block selection for the Comment Template block. We would appreciate your help there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comment Template Affects the Comment Template Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants