Experiment: Comments block: client-side form submission#53737
Draft
luisherranz wants to merge 15 commits intotrunkfrom
Draft
Experiment: Comments block: client-side form submission#53737luisherranz wants to merge 15 commits intotrunkfrom
luisherranz wants to merge 15 commits intotrunkfrom
Conversation
|
Size Change: +2.74 kB (+0.15%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0a31e54. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7232974945
|
This was referenced Aug 16, 2023
luisherranz
commented
Aug 24, 2023
luisherranz
commented
Aug 24, 2023
Comment on lines
+231
to
+236
| if ( | ||
| ( result === false || | ||
| result === undefined || | ||
| result === null ) && | ||
| attribute[ 4 ] !== '-' | ||
| ) { |
Member
Author
There was a problem hiding this comment.
This needs to be moved to its own PR and reviewed to match Preact's logic
luisherranz
commented
Aug 24, 2023
Comment on lines
+67
to
+70
| if ( $p->next_tag( 'textarea' ) ) { | ||
| $p->set_attribute( 'data-wp-bind--value', 'context.core.comments.text' ); | ||
| $p->set_attribute( 'data-wp-on--change', 'actions.core.comments.updateText' ); | ||
| } |
Member
Author
There was a problem hiding this comment.
We need to control the rest of the inputs (author, email…).
We can use the name attribute to find them because names should be stable among WordPress sites:
https://github.com/WordPress/wordpress-develop/blob/bf00a673a5f1d7f4707a6f914e1408526a8dc06a/src/wp-includes/comment.php#L3468-L3485
Something like this:
while ( $p->next_tag( 'input' ) ) {
if ( $p->get_attribute( 'name') === "..." )
}
luisherranz
commented
Aug 24, 2023
| } | ||
| ); | ||
|
|
||
| // data-wp-slot |
Member
Author
There was a problem hiding this comment.
We should move this to its own PR.
Member
Author
|
Ok. This is still the most basic version of this functionality, but it should be ready for the first reviews. There are still some small details to polish, but @DAreRodz is taking care of them and they should be ready in 1-2 days. Please take a look at the list of tasks in the PR description to know what is left, but they should not be an impediment to start reviewing this PR. There's also a bigger list of subsequent tasks that we want to do, but we will do them in separate PRs. @alexstine, @joedolson, @jeryj: Would you mind taking a look at the accessibility? This is how it currently works:
Please, let me know if something is missing or if we should change something. Again, thanks a lot 🙏🙂 |
jeryj
requested changes
Aug 24, 2023
Contributor
jeryj
left a comment
There was a problem hiding this comment.
Howdy y'all! Thanks for the a11y ping. I played around with it on Safari + VoiceOver. The announcements seem to work well.
It looks like this only works for top-level replies. I didn't see it noted that replies to comments would be included, so I wanted to make sure it was on the TODO list. That's most of the issues I found occur.
Duplicate "Comment submitted" announcement
- Using a screen reader (Safari + VoiceOver)
- Submit a comment
- "Comment submitted" announcement
- Press "Reply" link
- "Comment submitted" announcement is repeated
Move focus to Reply form
When clicking "reply", focus should get moved to the reply textbox.
Comment reappears after clicking "reply" to posted comment
- Post a comment
- Click on Reply from that comment
- Two forms are available, the reply to user and main thread comment forms.
- The main thread comment form is repopulated with the submitted message instead of being empty
| }; | ||
|
|
||
| const enhancedSubmissionNotice = __( | ||
| 'Enhanced submission might cause interactive blocks within the Comment Template to stop working. Disable it if you experience any issues.' |
Contributor
There was a problem hiding this comment.
Where and how do they disable it? Having more specific actionable error messages will be helpful. Also, what kinds of interactive blocks might stop working? I'm not sure what kinds of things to look out for.
Member
Author
There was a problem hiding this comment.
what kinds of interactive blocks might stop working?
This is tricky because the correct answer is "interactive blocks that are not using the Interactivity API", which is too technical and doesn't make sense to put in the UI. We're still trying to come up with ways to auto-detect those blocks, but we haven't come up with a 100% reliable technique yet. Once we do, we can remove that message.
Member
Author
There was a problem hiding this comment.
we haven't come up with a 100% reliable technique yet
I had an idea to make this 100% reliable and remove the warning. I'll open a discussion to talk about it. It also applies to the Query block pagination.
Member
Author
There was a problem hiding this comment.
There you go:
Any input would be highly appreciated 🙂
| initialOpen={ false } | ||
| > | ||
| <ToggleControl | ||
| label={ __( 'Enhanced form submission' ) } |
Contributor
There was a problem hiding this comment.
Will there be other enhancements as well? Otherwise, I'm wondering if there's a more specific name for this feature that tells you what it does. However, I'm trying to think of an improvement and not coming up with anything short and accurate 😅
packages/block-library/src/comments/edit/comments-inspector-controls.js
Outdated
Show resolved
Hide resolved
Member
Author
luisherranz
commented
Aug 24, 2023
| ( { props: { children } } ) => ( | ||
| <SlotProvider>{ children }</SlotProvider> | ||
| ), | ||
| { priority: 4 } |
Member
Author
There was a problem hiding this comment.
@DAreRodz: Is there a use case where data-wp-slot-provider and data-wp-slot can be used in the same component, or that doesn't make sense? (if there is, we should change their priorities so they can work in the same element)
Contributor
There was a problem hiding this comment.
It would make sense if you want to have a different slot context inside a fill. In that case, data-wp-slot-provider should be evaluated after data-wp-slot, so the provider should have less priority. 🤔
Same if you want to use data-wp-slot-provider and data-wp-fill together.
| const slot = evaluate( fill, { context: contextValue } ); | ||
| return <Fill slot={ slot }>{ children }</Fill>; | ||
| }, | ||
| { priority: 4 } |
Member
Author
There was a problem hiding this comment.
@DAreRodz: And shouldn't this priority be more than data-wp-context so they can be used in the same element? Why not use the normal 10 priority?
<div data-wp-context='{ "fill": "some fill name" }' data-wp-fill="context.fill">
I'm a fill and I can be repositioned by changing `context.fill`
</div>
Contributor
There was a problem hiding this comment.
As this directive adds a wrapper around the main element, I felt it would be better to give more priority to these directives than regular directives. But you're right; data-wp-context should be evaluated first if you want to use it in the slot container.
I'll consider it when I move the slot & fill implementation to its own PR.
Contributor
There was a problem hiding this comment.
As I mentioned in #53958 (comment):
[...] this is something I wanted to change. Surprisingly, the implementation stopped working as expected if I changed the priority order, so I decided to leave the current values for now.
For some reason, the Slot component behaves weirdly if I change the directive priorities. When it adds the slot before or after the main element, the Slot fallback is duplicated, and the Fill disappears.
I tried to figure out why that was happening without success, so I eventually decided to leave the current priority values, which are working fine. We can take a deeper look at this in the future.
cc: @luisherranz
luisherranz
commented
Aug 24, 2023
| `#${ newComment.id }` | ||
| ); | ||
|
|
||
| ref.reset(); |
Member
Author
There was a problem hiding this comment.
Once the fields are controlled, we cannot use this anymore and we should reset the values in the context instead.
1ab5449 to
0a31e54
Compare
Contributor
ockham
reviewed
Dec 16, 2023
| }, | ||
| }, | ||
| actions: { | ||
| submit: async ( event ) => { |
Contributor
There was a problem hiding this comment.
We still need to update this, see #56984 (comment):
This needs to use
*instead ofasyncandyieldinstead ofawait🙂
Member
Author
|
Thanks @ockham! 🎉 |
1 task
Contributor
|
Hello everybody! May I ask, if meanwhile someone came up with...
... to move this great PR forward? |
Member
Author
|
We have actively started working on that part. The first step has been taken in this pull request, although the initial logic of how to handle the assets is still very preliminary and needs more work: Our idea is that the same logic should also be applied to the region-based client-side navigation. This way, it would solve the problems that arise when a new block appears in the region and was not on the previous page, which is what stopped this PR. |
1 task
0a31e54 to
204fa39
Compare
Contributor
|
I was just pointed to this - looks like great work. Some input for your consideration in this Bluesky post and this blog post. I've also created this ticket for core to change how we deal with comment cookies, which actually might benefit you in this work as well. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
What?
Built on top of #53733. Supersedes #49305.
This PR adds an option to the Comments block to replace the current server-side form submission with a client-side one.
Why?
To improve the user experience.
How?
The form submission is intercepted using a directive (
data-wp-on--submit), which stops the regular form behavior and makes an HTTP request using JavaScript. If the submission goes through successfully, the comments list is updated with the HTML response. If the submission fails, the problem is shown on the screen.Tasks
There are still many tasks that need to be done before this can be considered ready. I'll probably add more as we progress and find out what needs to be done.
data-wp-navigation-idif the option is enabled.supports.interactivityand enqueue theview.jsfile in the Post Comments Form block if the option is enabled.data-wp-keyto the individual comments.replybuttons are clicked.replybutton.replybutton.cancel replybuttons when using thereplybutton.data-wp-bindlogic to its own PR. Interactivity API: Updatedata-wp-binddirective logic #54003data-wp-slot/data-wp-fillimplementation to its own PR. Interactivity API: add Slot and Fill directives #53958Tasks left for subsequent PRs
data-wp-dangerous-htmlinstead ofdata-wp-text.domoption tonavigatethat accepts the parsed DOM instead of thehtmland use that instead of thehtml.Testing Instructions
To test the initial implementation:
Testing Instructions for Keyboard
None yet.
Screenshots or screencast
Screen.Capture.on.2023-08-16.at.15-32-27.mp4