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

Updates placeholder for post comment block #27013

Merged
merged 2 commits into from Nov 19, 2020
Merged

Conversation

karmatosed
Copy link
Member

@karmatosed karmatosed commented Nov 16, 2020

This is a little PR that cleans up the placeholder on post comment block. Whilst a lot of these content type blocks are a work in progress, there are some visual hitches that can be smoothed. Primarily what this does:

  • Brings the input inline with height of other inputs (for example table placeholder).
  • Adds space between the input and save (as per other blocks).
  • Adjusts copy to be a little clearer what it does.

Before:

image

After:

after-postcomments

Feedback

I specifically would like a code review, but this shouldn't need much feedback as really just aligning with other blocks. Of course, any input is always welcome.

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

github-actions bot commented Nov 16, 2020

Size Change: -13 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-editor/index.js 133 kB -379 B (0%)
build/block-library/index.js 147 kB +25 B (0%)
build/components/index.js 171 kB +294 B (0%)
build/components/style-rtl.css 15.3 kB +6 B (0%)
build/components/style.css 15.3 kB +5 B (0%)
build/core-data/index.js 14.8 kB +3 B (0%)
build/data-controls/index.js 827 B +6 B (0%)
build/edit-post/style-rtl.css 6.51 kB -3 B (0%)
build/edit-post/style.css 6.49 kB -2 B (0%)
build/edit-site/index.js 23.3 kB +12 B (0%)
build/edit-site/style-rtl.css 3.98 kB +4 B (0%)
build/edit-site/style.css 3.98 kB +4 B (0%)
build/edit-widgets/style-rtl.css 3.16 kB +6 B (0%)
build/edit-widgets/style.css 3.16 kB +6 B (0%)
ℹ️ 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/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/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 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/compose/index.js 9.9 kB 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-widgets/index.js 26.3 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

@karmatosed
Copy link
Member Author

karmatosed commented Nov 16, 2020

Looping in @jameskoster here as potentially this block will get iterated outside of this PR. My thinking is this visual fix can ease whilst that's getting done, but happy to close now I know about #26856.

Ideally going beyond the visual fix bringing in an input field could be a nice change similar to the table block, but there's a visual win outside of that change here.

}

input {
height: 36px;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace 36px with $button-size here.

@jasmussen
Copy link
Contributor

Cool PR.

I think it needs a slightly different approach though. For a basic placeholder such as this one, you shouldn't need to create a new editor style just to fix this layout issue, those should be fixed at the component level, i.e. so every future placeholder like this works.

So I did a little inspecting, you can look here:

change

So it looks from the above that .components-placeholder__input[type=url] has the right CSS rules to make this look good, but that the input field used for the Post Comment placeholder, for some reason, does not inherit those rules. So either it's missing a classname (probably missing the components-placeholder__input class) or it's something else.

@karmatosed
Copy link
Member Author

karmatosed commented Nov 17, 2020

@jasmussen Thanks for the review. I did remove the styles and would love to fix at component level if that's possible. I did some digging and could manage to get the class to wrap everything, but not on specific element. I'd happily learnt that though in this case.

@jasmussen
Copy link
Contributor

Yep, looks like this uses the <TextControl> component, which is not aware of its placeholder context and therefore doesn't inherit the same rules as a plain old <input> field with a components-placeholder__input class attached. So it seems that needs a separate look-see. @ItsJonQ I wouldn't ask you to work on this — only share your thoughts on what the best way to systematize this is.

In the mean time, as a description change, we can probably just get this in.

@jasmussen jasmussen self-requested a review November 19, 2020 07:43
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

It doesn't fix the clunky input spacing, but it fixes the description.

@karmatosed
Copy link
Member Author

Yep, looks like this uses the component, which is not aware of its placeholder context and therefore doesn't inherit the same rules as a plain old field with a components-placeholder__input class attached. So it seems that needs a separate look-see. @ItsJonQ I wouldn't ask you to work on this — only share your thoughts on what the best way to systematize this is.

Ah! This explains why I couldn't get it to work way I expected whilst following definitions! No rush at all on this, I'll look at getting this merged as a string and then we can take from there.

@karmatosed karmatosed merged commit 5040f3d into master Nov 19, 2020
@karmatosed karmatosed deleted the try/post-comment-spacing branch November 19, 2020 10:10
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 19, 2020
@ItsJonQ
Copy link

ItsJonQ commented Nov 20, 2020

I wouldn't ask you to work on this — only share your thoughts on what the best way to systematize this is.

(Forgive me for stating obvious things. I'm just walking through all the steps)

Based on the original screenshot, it looks like there's a height difference between the controls (input x button).


⚛️ Systematic Control UI

From a systematic perspective, all control elements should have the same height value. They should also have a series of height variants (e.g. small -> large).

In the case of G2 Components, the cluster of input + button would look like this:

<Grid templateColumns="200px 1fr">
    <TextInput {...props} />
    <Button {...props} />
</Grid>

The sizes can be made larger through the size prop:

<Grid templateColumns="200px 1fr">
    <TextInput size="large" {...props} />
    <Button size="large" {...props} />
</Grid>

The Grid component will put these components in place. Their widths can be specified through the templateColumns prop, which maps to the CSS grid-template-columns property for CSS grid.


🛰 Spacing

You may notice there's nothing specified for spacing 🤔.

That's because Grid (and other multiple element layout components in G2) does this automatically.

For Grid, the space (or gap) between each element can be adjusted through the gap prop.


✨ Examples

I've prepared an example CodeSandbox with various examples.

https://codesandbox.io/s/g2-control-cluster-size-example-4blni

Hope this helps!

@gziolo gziolo added [Block] Comments (legacy) The legacy mode of the Comments block (formerly known as Post Comments) and removed [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop labels Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comments (legacy) The legacy mode of the Comments block (formerly known as Post Comments) Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants