Size Change: +265 B (+0.01%)

Total Size: 2.37 MB

_{compressed-size-action}

Flaky tests detected in a555502.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18843942045
📝 Reported issues:

• [Flaky Test] should refresh the page when stylesheet loading fails #71722 in /test/e2e/specs/interactivity/router-styles.spec.ts

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: jasmussen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

This is working fairly well in my testing

Remaining issues:

• when the user enters text into the comment box and its size grows to accommodate the longer text, the floating notes below the new note form do not move down to make room. To fix this, I'll need to add a debounced handler to reflow the comments similar to what we did in the other form.
• after opening the new comment popup, selecting a different block leaves an artifact

• after opening the new comment popup, selecting a different block leaves an artifact

This is fixed in 549838f

when the user enters text into the comment box and its size grows to accommodate the longer text, the floating notes below the new note form do not move down to make room.

This is fixed in dcf659f

This is close, I still see one issue where the new comment block is not placed in the right position. I will work on addressing.

Interesting approach, @adamsilverstein!

As you said, it needs some polishing, but it is also working :)

Thanks for the review and feedback. My initial attempt at this was to make the AddComment component itself float, however given the need to keep the threads from overlapping, I found the current approach to be way more reliable.

I'll work on the code polish you suggested. The main remaining glitch I see when testing this out is after opening the new note popup, selecting a different block without an existing comment should move the new comment form to be aligned with that block, currently it does not.

This is testing fairly well for me, and I can't break it:

Notably I like that there's animation when the canvas scales down to make room for a note.

And in that similar vein, but not something that has to be fixed in this cycle, between the "input note" being submitted, and the final note appearing, there's a small blink where the note disappears, then reappears. I imagine there are foundational technical reasons for why this is so, but conceptually it is the same note, so a bit of delight would be if the note input itself faded in (quickly, e.g. 0.1 or 0.2s speed), and didn't blink upon submission, perhaps simply resized itself as the input becomes text and the threading buttons below appear.

Is that going to eventually be possible, and if yes, something worth tracking separately?

Notably I like that there's animation when the canvas scales down to make room for a note.

Props to using sidebars; we get that animation for free.

but conceptually it is the same note

I agree that a similar animation would be really nice.

P.S. While visually it appears to be the same note, technically the component rebuilds the entire notes tree and renders the newly added note. I would probably file this into the "nice to haves" list.

Pleaking of blinks. When clicking the "Add note" button, the form sometimes blinks to the top and then renders correctly. It's not a blocker; something would be nice to resolve in follow-up.

Here, I had to throttle the CPU to make it somewhat visible in the screencast.

This is testing fairly well for me, and I can't break it:

🎉 Woohoo!

Thanks for the review @jasmussen and for the refactoring @Mamaduka - I'll give this some further testing!

Sorry for the late, checking the code and discussions now 👀

I pushed the following changes.

f83dd68: I removed tabIndex:0 from the new note form. I think the new note form doesn't need to be focusable at all, since the thread itself is not expandable.

33563df:

Removed unnecessary VStack components. This will fix some visual issues and layout problems.

Before	After

f83dd68: I removed tabIndex:0 from the new note form. I think the new note form doesn't need to be focusable at all, since the thread itself is not expandable.

I think we need to restore this. Otherwise, clicking inside the add new note element triggers onBlur and closes it.

Thanks for the refinements here @t-hamano!

This continues to work well in my testing, once the tests pass we can merge.

I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: 2060336