-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Experiment: Comments block: client-side form submission #53737
base: trunk
Are you sure you want to change the base?
Experiment: Comments block: client-side form submission #53737
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
|
if ( | ||
( result === false || | ||
result === undefined || | ||
result === null ) && | ||
attribute[ 4 ] !== '-' | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be moved to its own PR and reviewed to match Preact's logic
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' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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') === "..." )
}
@@ -278,4 +285,56 @@ export default () => { | |||
} ); | |||
} | |||
); | |||
|
|||
// data-wp-slot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this to its own PR.
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 🙏🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -17,8 +24,21 @@ export default function CommentsInspectorControls( { | |||
"The <aside> element should represent a portion of a document whose content is only indirectly related to the document's main content." | |||
), | |||
}; | |||
|
|||
const enhancedSubmissionNotice = __( | |||
'Enhanced submission might cause interactive blocks within the Comment Template to stop working. Disable it if you experience any issues.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you go:
Any input would be highly appreciated 🙂
initialOpen={ false } | ||
> | ||
<ToggleControl | ||
label={ __( 'Enhanced form submission' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
( { props: { children } } ) => ( | ||
<SlotProvider>{ children }</SlotProvider> | ||
), | ||
{ priority: 4 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
`#${ newComment.id }` | ||
); | ||
|
||
ref.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the fields are controlled, we cannot use this anymore and we should reset the values in the context instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3cdc365
Sure, happy to! |
1ab5449
to
0a31e54
Compare
}, | ||
}, | ||
actions: { | ||
submit: async ( event ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to update this, see #56984 (comment):
This needs to use
*
instead ofasync
andyield
instead ofawait
🙂
Thanks @ockham! 🎉 |
Hello everybody! May I ask, if meanwhile someone came up with...
... to move this great PR forward? |
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. |
0a31e54
to
204fa39
Compare
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-id
if the option is enabled.supports.interactivity
and enqueue theview.js
file in the Post Comments Form block if the option is enabled.data-wp-key
to the individual comments.reply
buttons are clicked.reply
button.reply
button.cancel reply
buttons when using thereply
button.data-wp-bind
logic to its own PR. Interactivity API: Updatedata-wp-bind
directive logic #54003data-wp-slot
/data-wp-fill
implementation to its own PR. Interactivity API: add Slot and Fill directives #53958Tasks left for subsequent PRs
data-wp-dangerous-html
instead ofdata-wp-text
.dom
option tonavigate
that accepts the parsed DOM instead of thehtml
and 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