Skip to content
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

Adds in styling for post comments form #27009

Closed
wants to merge 1 commit into from

Conversation

karmatosed
Copy link
Member

@karmatosed karmatosed commented Nov 16, 2020

This is an initial PR, but will need some iteration based on feedback, provides a step towards a solution for #26865.

Before:

before

After:

styled-form

Showing the block style and non-block style:
In the PR this explicity is styled only around the block class, but this might not be desireable so noting this here. This theme is experimental so please ignore the message about comments.php seen, I am simply using it to illustrate both.

both-forms

Mobile:

mobile

Feedback

There are a few things not done in this PR and a few questions, the code also needs a review.

  • Do we want 2 potentially different styles of post comment form on a page? As you can see you can have this, the cases of this are rare as most would turn off comments if put in page, but it's worth raising as condition.
  • Do we want to have the style not just for the block? There's a danger there of theme complications hence why I wrapped in block and think it's best.
  • In the initial mocks the border was using the wp-admin blue for submit button, in seeing this PR I would lean towards black. What do others think?
  • As this uses the post comment form, pulling out the button wasn't an easy to find task. If this is desireable I would appreciate someone stepping in there.

Along with this I would love some feedback, if nothing else this PR showed the limits of what was possible and the CSS needed to get there for this block.

@karmatosed karmatosed added Needs Design Feedback Needs general design feedback. [Block] Post Comments Form Affects the Comments Form Block labels Nov 16, 2020
@karmatosed karmatosed self-assigned this Nov 16, 2020
@github-actions
Copy link

Size Change: +249 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/theme-rtl.css 916 B +124 B (13%) ⚠️
build/block-library/theme.css 918 B +125 B (13%) ⚠️
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.92 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.9 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.51 kB 0 B
build/edit-post/style.css 6.49 kB 0 B
build/edit-site/index.js 23.3 kB 0 B
build/edit-site/style-rtl.css 3.98 kB 0 B
build/edit-site/style.css 3.98 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@MaggieCabrera
Copy link
Contributor

Thank you for this PR, we were discussing this last week @scruffian

Should we also consider the styles for the form when the user is not logged in on this PR?

Screenshot 2020-11-27 at 13 50 17

@karmatosed
Copy link
Member Author

@MaggieCabrera thanks for your feedback.

Should we also consider the styles for the form when the user is not logged in on this PR?

Probably if we do provide styling it should be done for all states. This is where though finding how far we want to go with the default styling is a great conversation to be had.

@MaggieCabrera
Copy link
Contributor

Probably if we do provide styling it should be done for all states. This is where though finding how far we want to go with the default styling is a great conversation to be had.

I like the direction your PR is going and I believe this should translate to the frontend as well. During our week experimenting with block-based themes, we were using this boilerplate theme as a base and we noticed how broken the comments were. In my opinion, we should provide a base for the themes to work upon, not have them built it from scratch every single time.

width: 100%;
}

#submit{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this implement a button block?

@scruffian
Copy link
Contributor

+1 to what @MaggieCabrera said. I think it's also important to consider what parts of this users will want to customize themselves. I wonder if it makes sense for lots of these elements to be child blocks so they can benefit from the same customizations that other blocks have.

Base automatically changed from master to trunk March 1, 2021 15:44
@carolinan
Copy link
Contributor

Hi @karmatosed, are you interested in picking this up again?
The button block CSS classes are now used for the submit button so you wont need to style that.

@karmatosed
Copy link
Member Author

@carolinan happy to, I can look at it this week.

Copy link
Member

@colorful-tones colorful-tones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR is quite outdated and @karmatosed is likely to be taking another pass soon. So, please ignore my code review comments if no longer pertinent.

Also, worth noting that there may some minor overlap here with Global Styles: Form elements #29167

@@ -0,0 +1,30 @@
.wp-block-post-comments-form {

h3#reply-title {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this specificity?

h3#reply-title {
margin-top: $grid-unit-20;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - avoid element selector if possible please?

@karmatosed
Copy link
Member Author

I am having quite some time getting this PR working right now so going to close and create new one as this got incredibly stale. I'll link a fresh version though - turns out a lot happens in nearly a year to make PRs out of date (which is awesome).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Comments Form Affects the Comments Form Block Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants