Remove core/post-comments-form block styles that are overriding button element styles #42053
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
…hey are overriding the core/block button styles.
matiasbenedetto
added
[Type] Bug
|
Size Change: +6.79 kB (+1%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Jun 29, 2022
There was a problem hiding this comment.
I think we should only remove the styles that are provided by core theme.json. For example cursor:pointer is not, so it should remain. Testing this I saw no difference because the class .wp-block-button__link in the button block's CSS adds cursor:pointer, but IMO it should not be removed - same to all other CSS that are not in theme.json.
matiasbenedetto
force-pushed
the
fix/42052
branch
from
e86b893
to
c5429d9
Compare
|
I updated the testing instructions following the issue conversation. |
|
Using Empty theme. Before: With Gutenberg trunk, and changes copied from PR description and added to theme.json: After, with PR applied: Without button styles in theme.json, the button has the browser default border for buttons (and I assume this is the style that will be the default style used in classic themes).
There is also an issue with the border radius, but I do not know which of the button element PR's that caused it. I suspect that this might even bleed through from the WordPress admin area styles, from the .submit CSS class? |
|
I am not fond of the browser default border that will be used if theme authors do not style this button, but I also do not have a solution for it. |
draganescu
previously requested changes
Jun 30, 2022
There was a problem hiding this comment.
Let's wait on figuring out what's up with the roundness in @carolinan 's tests
matiasbenedetto
changed the title
|
The
|
…e.scss file to make them available on the frontend.
…o avoid unwanted styles
|
The problems around border-radius found by @carolinan preexist this PR and they are not strictly related to the original intention of this PR. Apart from that, I added a default setting in Gutenberg's default In this screenshot, you can see that both problems seem to be solved. For this example, I'm using
With Gutenberg plugin disabled still are some visual discrepancies between the editor and the frontend caused by some admin CSS definitions. I may have a fix for that, I'm testing it. Due it is a core-problem that needs to be fixed in core repo I think we can move forward with this PR. Thanks @carolinan and @draganescu for your feedback! |
I think that class pre-dates that PR... |
draganescu
reviewed
Jul 6, 2022
| @@ -34,7 +34,6 @@ const CommentsForm = () => { | |||
| name="submit" | |||
| type="submit" | |||
| className={ classnames( | |||
| 'submit', | |||
There was a problem hiding this comment.
Where else is this class? If it's not needed should we remove the directives as well?
There was a problem hiding this comment.
I haven't found the class anywhere else in Gutenberg codebase.
The CSS directives live in the WordPress core repo: https://github.com/WordPress/wordpress-develop/blob/0a17a80bccd452b91c3b63f71f010ba131a2c954/src/wp-admin/css/forms.css#L415-L421
draganescu
dismissed
their stale review
The new updates seem to solve the problems
matiasbenedetto
added
the
[Block] Post Comments Form
michalczaplinski
approved these changes
Jul 15, 2022
There was a problem hiding this comment.
This looks good to me 👍
I have originally added the submit classes in 08d34f3 when creating the Post Comments Form for the editor. The only reason for adding it was because the PHP (non-block) comments also include it.
michalczaplinski
requested changes
Jul 15, 2022
There was a problem hiding this comment.
Uh, I'm sorry for having to revoke my approval 😅
I have noticed that the submit class is also applied on the frontend through the comment_form_defaults filter:
This is where the comment_form_defaults filter is being applied in Core.
This filter was introduced to the Post Comments Form block in #40628 because of the feedback in #34994 (comment).
So, I think we should either remove the submit class in both places or keep it in both 🙂 .
|
Thanks for your detailed feedback @michalczaplinski ! |
michalczaplinski
approved these changes
Jul 15, 2022
There was a problem hiding this comment.
I think that's fine! 🙂
There is only one class (submit) applied to the button which is defined here and added to the final markup here
Also, I wanna note this comment which confirms that CSS classes can change from time to time given good reasons (I'm paraphrasing).
37 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?
Remove core/post-comments-form block styles that are overriding button element styles from
theme.jsonWhy?
These styles are preventing that the
theme.jsonstyles for button elements apply over the core/post-comments-form submit button.How?
I think those styles can be removed because they are no longer needed and they are conflicting with the button element styles.
Testing Instructions
Screenshots or screencast
Using the
theme.jsonfrom the top ⬆️ example:Before:
After:
