Components: Remove global button styles from Post Comment Actions

[sarayourfriend]

Changes proposed in this Pull Request

• Use the Button component for the comment action buttons

Testing instructions

• Go to the reader and open a post's comments section. check that the buttons work and look like they do in production.

Related to #45259

[matticbot]

Test live: https://calypso.live/?branch=refactor/global-button-styles-comment-actions

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~34 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest       +138 B  (+0.2%)      +34 B  (+0.2%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~148 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-main               +2666 B  (+0.2%)      +38 B  (+0.0%)
entry-gutenboarding        +46 B  (+0.0%)     +110 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~17579 bytes added 📈 [gzipped])

name             parsed_size            gzip_size
reader              +67620 B  (+13.2%)   +17494 B  (+13.0%)
post-editor            +46 B   (+0.0%)      +63 B   (+0.0%)
jetpack-connect        +46 B   (+0.0%)      +22 B   (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~15911 bytes added 📈 [gzipped])

name                                         parsed_size            gzip_size
async-load-blocks-app-promo                     -67600 B  (-88.7%)   -17467 B  (-85.8%)
async-load-design-blocks                        +67128 B   (+2.2%)   +16307 B   (+2.3%)
async-load-components-web-preview-component     +64718 B  (+13.3%)   +17153 B  (+12.9%)
async-load-reader-sidebar                         -203 B   (-0.4%)      -82 B   (-0.6%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[tyxla]

There seems to be a subtle difference between the original spacing above/below the comment action buttons, and the one in this branch.

Before:

After:

As you can see, with the spacing in this PR, comment action buttons aren't vertically centered, while originally they are more or less vertically centered.

[tyxla]

Let's add a newline at the end of the file.

[jsnajdr]

There seems to be a subtle difference between the original spacing above/below the comment action buttons

This patch adds the .button class to the comment action buttons, and there are two CSS properties that modify the spacing.

After the patch, the space above the button row is smaller, i.e., the button row is closer to the text above it. That's because of:

.button .gridicon {
  margin-top: -2px;
}

which adds negative margin on top of the gridicon element, making the overall .button box height smaller. As the box text baseline is aligned with the bottom edge of the box, it removes whitespace above the text.

After the patch, the space below the button row is bigger, i.e., there's more space above the next comment. That's because of:

.button {
  line-height: 22px;
}

The button has a bigger line-height that it used to. This, in theory, adds more space both above and below the baseline. In this case, it adds space only below the baseline, probably because the part of the box above the baseline is already tall enough because of the gridicon element.

If we want to rebalance the spacing again, the best solution seems to be adding a margin-top to the .comments__comment-actions container.

[jsnajdr]

Is this extend really needed? I would expect the element get all the needed styles from the .button class.

Looking at devtools, I see some differences in, e.g., font-size and padding. But I'm a bit confused by all that rather than understanding the intent:

[sarayourfriend]

Thank you for pointing this out. I was indeed going about this the wrong way. I've updated the branch with new code that doesn't rely on the global styles.

The problem I was running into was that I was removing the global styles to check how my changes were working, and getting confused with the like button, which was using a different method of getting the button tag. I've added something of a hack to make it work using the Button component, but I'm not sure if there's a better way to do this. I've never worked with this tagName pattern in React before, so I'm kind of just guessing based on what I can imagine will work based on the implementation I am seeing. I would especially love feedback with respect to that.

[sarayourfriend]

@tyxla I believe there are still slight differences, but I think the new version is actually more consistent with respect to how the icons are lined up. Let me know if you think there's still an issue though and I can try to match it more exactly.

[tyxla]

Thanks for your continuous work on this one 👍 I wouldn't want to block this PR any further, but I've noticed 2 more things I'd like to talk through before approving it:

• Spacing-wise, it's not ideal, but I think it's indeed closer to what we want. One thing I'm not super happy about is the spacing on the left - it doesn't align with the comment text above, so we might want to remove that left padding on the read more button. What are your thoughts?
• Color-wise, there's a difference between production and this branch for the read more button.

Production:

This PR:

Is that change intentional?

[jsnajdr]

@saramarcondes I think we're dealing with three kinds of buttons here:

1. button with native browser styles that make it look like, well, a button 🙂
2. Calypso <Button> that adds additional styling on top of the browser native ones (without any reset) and makes the button still look like button, but a Calypso-styled one (borders, colors, paddings, font, focus rings...)
3. button element that is styled to look like a regular div, i.e., one that resets the native browser styles. That's useful at places where we want button behavior (with all the a11y benefits it provider over a <div onClick>), but none of the styles. The comments actions buttons are good example of this: they're styled to look more like text links.

The %button extend you had in the previous version of the patch provided these button-to-div resets.

<Button borderless> is close to the button-as-div look, but not quite there. It's intended to still have the same dimensions, paddings and fonts like the regular button, just without the border. See how the borderless and border-ful buttons nicely align in this toolbar screenshot:

A button-as-div wouldn't do that. The .comments__comment-actions button.button CSS rule needs to do several somewhat random overrides to get from borderless to the desired button-as-div state.

Ideally, I think we need something like <Button plain> or <Button bare> to get that button-as-div styling with the reset. Such a button wouldn't have the .button CSS class, as that's already used for the Calypso-styled button. Instead, there could be a .button-plain CSS class that does the reset -- it would have the same contents as %button had.

I don't want to block this PR, once it gets good enough to ship, until we implement some perfect solution, but let's discuss if what I write above makes sense and if it's a viable roadmap to make the Calypso buttons a bit better.

@diegohaz do you have any insights about how to do the button-as-div components and how, e.g., Reakit or @wordpress/components approach this?

[jsnajdr]

Some nits:

• I'd call it just BorderlessCompactButton. It's a component rather that a tag. tagName, just like React.createElement, accepts either a component (function) or a HTML tag name (string)
• children is one of the props, no need to destructure it separately. ( props ) => <Button { ...props } borderless compact /> works, too.
• let's use BorderlessCompactButton consistenly across this module, using it everywhere instead of <Button borderless compact>

[jsnajdr]

This style conflicts with the one that gives the "Read more" button the primary color:

.comments__comment-actions .comments__comment-actions-read-more {
	color: var( --color-primary );
}

That causes the coloring issue that @tyxla reports in #45344 (review)

[sarayourfriend]

@jsnajdr Thanks for the detailed thoughts. I agree with you that we should take a different approach for this. I've added the bare style in this PR as you suggested and will revisit this PR once that is merged: #45386

[tyxla]

Looks and works well to me, and the code looks good.

On a side note, I'm not sure bare makes complete sense in terms of being predictive, obvious and clear what it does. What alternatives of bare can we find? Material design calls those "text buttons". while Bootstrap calls them Links or Link Buttons (it also has a Jumbotron component, so not sure we wanna borrow naming ideas from there, lol). Chakra UI also calls them links FWIW.

What are your thoughts on the prop naming?

[tyxla]

FWIW, consider the bare discussion irrelevant to this PR's blocked state. From my perspective, this is good to go already.

[jsnajdr]

What are your thoughts on the prop naming?

I'd rule out "link buttons" because that suggests the component is an <a href> link with button styling. While it's exactly the opposite: it's a <button> without button styling 🙂 <Button text> or <Button plain> look like good alternatives to me.

[tyxla]

I agree on all points @jsnajdr. While text is a bit ambiguous, plain sounds great to me FWIW 👍

[diegohaz]

@diegohaz do you have any insights about how to do the button-as-div components and how, e.g., Reakit or @wordpress/components approach this?

Reakit doesn't care about styling. The closest concept is the as prop that can be used to change the underlying element. It's not about styling, but how the element will behave. You can do something like <Button as="div">, which would render a <div> element with all the necessary behaviors so it remains accessible:

• role="button" and tabindex attributes are added to the div.
• Pressing Enter triggers click on the div on key down.
• Pressing Space activates the button (similar to :active) on key down and triggers click on key up.
• disabled adds aria-disabled and disables pointer and keyboard events on the div.

In @wordpress/components, a Button is by default a pretty simple element with most browser styles reset, including appearance: https://wordpress.github.io/gutenberg/?path=/story/components-button--default

It has props like isPrimary, isSecondary, isTertiary. And they are responsible for making it look more like a WordPress button.

The polymorphism there is resumed to the button and a elements.

[sarayourfriend]

@tyxla I'd love a re-review after having replaced the Automattic component button with the WordPress components equivalent. Let me know what you think.

[tyxla]

Thanks for using @wordpress/components instead, @saramarcondes!

FWIW, currently in calypso, we don't add the @wordpress/components stylesheet, so any WordPress components you use won't have their corresponding styles loaded.

#45724 is taking care of that, so maybe we should rebase and test this one again after we land #45724. What do you think?

[sarayourfriend]

@tyxla Oh gosh, I feel very silly! Yes, that would make sense to do first, wouldn't it 🤦‍♀️ It didn't even occur to me that we needed to load the stylesheet separately or that it would be affecting these PRs. Not sure how I didn't connect those dots.

Lets land #45724 like you said and then revisit this one.

[tyxla]

@saramarcondes I've rebased the PR since we merged #45724 and it seems like the buttons work really well!

The only thing we needed to do is to reset the height that's coming by default for @wordpress/components buttons. I've taken care of this in 982ea2d and this PR is good to go from my perspective.

It's worth noting that I've also tested the moderation actions, which are currently disabled in the UI. Regardless, they work just fine with this PR.

[tyxla]

LGTM! Nice work 👏

[tyxla]

Going to help ship this once tests are green.