Improve the logic for warnings for Post Comments Form placeholder #40563
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
the
[Block] Post Comments Form
|
Size Change: +51 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
| : false | ||
| ); | ||
|
|
||
| let warning = false; |
There was a problem hiding this comment.
By default we don't display a warning, like we do in the Post Comments block:
gutenberg/packages/block-library/src/post-comments/edit.js
Lines 53 to 55 in 54fd3bf
As the Post Comments Form is included in the Comments Query Loop, it would not make sense to say that you should "replace it with the Comments Query Loop". 😅
The form rendered in the editor should also be close enough style-wise to the one rendered on the frontend so that we don't need to warn about it.
|
I have tested the PR and it works as described. What is the expected result in the template editor? |
carolinan
approved these changes
Apr 25, 2022
luisherranz
approved these changes
Apr 25, 2022
There was a problem hiding this comment.
LGTM!
I guess you discarded sharing the same logic for both Post Comments and Post Comments Form blocks, didn't you?
If I open the template in the template editor, the block respects the settings for the current page, which I believe is correct
Oh, I didn't consider or test that in the logic flow. I'm glad it's working, though. I guess it is populating context.postType and context.postId then 🙂
ockham
force-pushed
the
fix/update-warning-logic-post-comments-form
branch
from
12053bb to
ec11afa
Compare
luisherranz
requested changes
Apr 25, 2022
luisherranz
approved these changes
Apr 25, 2022
|
Thanks @michalczaplinski for the PR and @carolinan and @luisherranz for reviewing! 🙏
We can do this in a follow-up. I had one more thought: In a follow-up, it'd be cool if we gave the user the possibility to resolve the issues that keep the Post Comments Form from working. I.e. if comments are disabled for a post or page with a certain ID, we could add a button to enable them; or if they're disabled for an entire post type, we could add a button that either enables them right away, or at least takes them to the relevant settings screen. (It's much less frustrating for a user if they're presented with a problem and a potential solution, rather than just the problem, and they're on their own to figure out the solution.) FWIW, the
|
|
Thanks everyone for the review. Will merge it now 👍
Yes, that's right! 🙂 It didn't seem like the right abstraction because the warnings would have slightly different wording and also differ in at least one place like mentioned in #40563 (comment)
That's a very good point. Will add it in a follow up. |
michalczaplinski
deleted the
fix/update-warning-logic-post-comments-form
branch
|
Another instance of a gutenberg/packages/block-library/src/comments-pagination/edit.js Lines 81 to 85 in c5b9ca1 (Noting here for now so I won't forget before we find time to file a proper issue 😅 ) |
|
I've filed #40614 to keep track of the Warnings that need buttons added. |
scruffian
pushed a commit
that referenced
this pull request
May 9, 2022
Root vars shouldn't apply to block global styles Fix issue with shorthand padding on server side. Temp fix for file path error Fix site editor top padding not updating Support shorthand properties in site editor. Fix full widths in the post editor Check selector inside function. Fix kebab-casing of CSS variables. Fix borked merge conflict solving Move post editor full width styles to edit-post package. Set default padding value to 0. Update test string. Fix PHP unit tests Fix PHP lint Fix failing PHP tests. Use block gap variable as default root padding value. Fix linting errors. Add opt-in setting via package.json Generate correct block editor styles Fix tests and add prop to core theme.json Fix unit tests properly this time Improve the logic for warnings for Post Comments Form placeholder (#40563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <[email protected]> Add optional chaining in case taxonomy visibility is not defined (#40532) Co-authored-by: Glen Davies <[email protected]> Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" Add alignments rules for blocks within post content Remove default padding match alignment rules to those used in blockbase move root padding rules to a new method revert unconnected change revert spacing change add docblock for new method remove file' fix merge mess formatting
scruffian
pushed a commit
that referenced
this pull request
May 11, 2022
Root vars shouldn't apply to block global styles Fix issue with shorthand padding on server side. Temp fix for file path error Fix site editor top padding not updating Support shorthand properties in site editor. Fix full widths in the post editor Check selector inside function. Fix kebab-casing of CSS variables. Fix borked merge conflict solving Move post editor full width styles to edit-post package. Set default padding value to 0. Update test string. Fix PHP unit tests Fix PHP lint Fix failing PHP tests. Use block gap variable as default root padding value. Fix linting errors. Add opt-in setting via package.json Generate correct block editor styles Fix tests and add prop to core theme.json Fix unit tests properly this time Improve the logic for warnings for Post Comments Form placeholder (#40563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <[email protected]> Add optional chaining in case taxonomy visibility is not defined (#40532) Co-authored-by: Glen Davies <[email protected]> Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" Add alignments rules for blocks within post content Remove default padding match alignment rules to those used in blockbase move root padding rules to a new method revert unconnected change revert spacing change add docblock for new method remove file' fix merge mess formatting fix merge mess fix alignment append to ruleset fix issue in site editor match the styles for the site editor to those used in the front end formatting change move new code to 6.1 compat files fix linter
scruffian
pushed a commit
that referenced
this pull request
May 16, 2022
Root vars shouldn't apply to block global styles Fix issue with shorthand padding on server side. Temp fix for file path error Fix site editor top padding not updating Support shorthand properties in site editor. Fix full widths in the post editor Check selector inside function. Fix kebab-casing of CSS variables. Fix borked merge conflict solving Move post editor full width styles to edit-post package. Set default padding value to 0. Update test string. Fix PHP unit tests Fix PHP lint Fix failing PHP tests. Use block gap variable as default root padding value. Fix linting errors. Add opt-in setting via package.json Generate correct block editor styles Fix tests and add prop to core theme.json Fix unit tests properly this time Improve the logic for warnings for Post Comments Form placeholder (#40563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <[email protected]> Add optional chaining in case taxonomy visibility is not defined (#40532) Co-authored-by: Glen Davies <[email protected]> Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" Add alignments rules for blocks within post content Remove default padding match alignment rules to those used in blockbase move root padding rules to a new method revert unconnected change revert spacing change add docblock for new method remove file' fix merge mess formatting fix merge mess fix alignment append to ruleset fix issue in site editor match the styles for the site editor to those used in the front end formatting change move new code to 6.1 compat files fix linter
scruffian
pushed a commit
that referenced
this pull request
May 18, 2022
Root vars shouldn't apply to block global styles Fix issue with shorthand padding on server side. Temp fix for file path error Fix site editor top padding not updating Support shorthand properties in site editor. Fix full widths in the post editor Check selector inside function. Fix kebab-casing of CSS variables. Fix borked merge conflict solving Move post editor full width styles to edit-post package. Set default padding value to 0. Update test string. Fix PHP unit tests Fix PHP lint Fix failing PHP tests. Use block gap variable as default root padding value. Fix linting errors. Add opt-in setting via package.json Generate correct block editor styles Fix tests and add prop to core theme.json Fix unit tests properly this time Improve the logic for warnings for Post Comments Form placeholder (#40563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <[email protected]> Add optional chaining in case taxonomy visibility is not defined (#40532) Co-authored-by: Glen Davies <[email protected]> Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" Add alignments rules for blocks within post content Remove default padding match alignment rules to those used in blockbase move root padding rules to a new method revert unconnected change revert spacing change add docblock for new method remove file' fix merge mess formatting fix merge mess fix alignment append to ruleset fix issue in site editor match the styles for the site editor to those used in the front end formatting change move new code to 6.1 compat files fix linter
adamziel
pushed a commit
that referenced
this pull request
Jun 21, 2022
…0563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <[email protected]>
14 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.
What?
Followup to #40368
In some cases, we want to show a warning to inform the user that one of the following is the case:
Here, we're improving the logic responsible for showing warnings in the Post Comments Form placeholder according to #40484 (comment)
Why?
Previous implementation was not specific enough.
How?
Mostly re-using the logic for showing the comments that was implemented for the Post Comments in #40484
Testing Instructions
Site Editor:
Post Editor:
Comments are not enabled on the site:
Post Comments Form block: Comments on this post are not allowed.warning should appear.Comments are not enabled for this post
Post Comments Form block: Comments are not enabled.warning should appear.Comments are not enabled for this custom post type
Post Comments Form block: Comments for this post type <post type name> are not enabledwarning should be visible.