Chore: style Gutenberg file block #773

derekshirk · 2020-06-03T17:39:24Z

Overview

This PR introduces styles for Gutenberg file blocks. File blocks can be displayed in one of two ways, as a "download link" or as a "download button".

The changes being introduced here, style both links and buttons with primary, secondary and tertiary button styles.

Screenshots

Example of the WP Gutenberg (file) block editor interface, demonstrating "download links" and "download buttons"

Styled WordPress output

Example of Storybook documentation

Testing

View deploy preview
Confirm links in story description are linked correctly
Review CSS
Confirm active, focus and Hover are working as expected

Issue

See #710

…atterns into chore/gutenberg-core-file

changeset-bot · 2020-06-03T21:36:05Z

🦋 Changeset is good to go

Latest commit: 24e0515

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…atterns into chore/gutenberg-core-file

calebeby · 2020-06-03T23:52:39Z

@derekshirk Can you add a changeset for this PR? I think this should go as a minor release since it adds functionality in a non-breaking way. We are releasing every PR with outwards-facing changes as its own release.

npx changeset

If you have any questions, let me know!

derekshirk · 2020-06-04T23:39:20Z

@cloudfour/dev This is still in need of review.

spaceninja

Left some nitpicky code formatting questions, but overall I have no objections.

src/vendor/wordpress/_wordpress.scss

spaceninja · 2020-06-04T23:52:56Z

src/vendor/wordpress/_wordpress.scss

+  &.c-button--tertiary .wp-block-file__button,
+  &.c-button--tertiary :not(.wp-block-file_button) {
+    @include button.tertiary;
+    color: inherit !important; /* 1 */
+    opacity: inherit !important; /* 1 */
+  }


Would it make sense to merge this with the previous block for c-button--secondary? That might avoid some duplication.

@spaceninja,

Would it make sense to merge this

When you mention "this", are you referring to the color and opacity values, or the entire block? 🤔

This is how I interpreted your suggestion to consider "merging with the previous block.

&.c-button--secondary .wp-block-file__button, &.c-button--secondary :not(.wp-block-file__button) { @include button.secondary; } &.c-button--tertiary .wp-block-file__button, &.c-button--tertiary :not(.wp-block-file_button) { @include button.tertiary; } /** * 1. Prevents default WP styling from modifying buttons opacity and * text color. */ &.c-button--secondary .wp-block-file__button, &.c-button--secondary :not(.wp-block-file__button), &.c-button--tertiary .wp-block-file__button, &.c-button--tertiary :not(.wp-block-file_button) { color: inherit !important; /* 1 */ opacity: inherit !important; /* 1 */ }

However, I don't have a high level of confidence that this is what you intended.

You can ignore my comment above, if you'd like. 😉

Excuse my additional, "think-out-loud" comments on this thread...

I've ended up with this:
(These comments do not exist in source)

.wp-block-file { ... /** * If I don't include `:not(.wp-block-file__button)` here and opt for `*` instead, * this selector is not specific enough to enforce the desired styles, as rules from the * `@wordpress/block-library` override various properties. */ .wp-block-file__button, :not(.wp-block-file__button) { @include button.default; opacity: inherit !important; } /** * If I don't include `wp-block-file__button` here, this selector is not * specific enough to enforce the desired styles, as rules from the * `@wordpress/block-library` override various properties. * * 1. I tried _only_ using the `*` selector here but ran into the same specificity issue. * 2. If I'm forced to use `:not(.wp-block-file__button)`, I considered using it here * as well in hopes of perceived consistency */ &.c-button--secondary { .wp-block-file__button, * { @include button.secondary; color: inherit !important; } } /** * If I don't include `wp-block-file__button` here, this selector is not * specific enough to enforce the desired styles, as rules from the * `@wordpress/block-library` override various properties. * * 1. I tried _only_ using the `*` selector here but ran into the same specificity issue. * 2. If I'm forced to use `:not(.wp-block-file__button)`, I considered using it here * as well in hopes of perceived consistency */ &.c-button--tertiary { .wp-block-file__button, * { @include button.tertiary; color: inherit !important; } } }

In response to the original concern of duplication, I was able to remove the opacity property from two selectors. Regarding the remaining color property, it was not my findings that this produced unnecessary duplicative CSS output.

That looks great! I'll be honest, in my original comment I intended you to combine the secondary and tertiary blocks, but that was because I missed that they were including different styles. My bad. Your changes combined with these comments look good.

Thanks @spaceninja

src/vendor/wordpress/_wordpress.scss

spaceninja · 2020-06-04T23:59:14Z

src/vendor/wordpress/gutenberg.stories.mdx

-This block creates a `div` with class `wp-block-file`, which wraps one or two
-`a` elements, the second of which (toggled in the editor) is meant to be styled
-as a button, adding the class `wp-block-file__button`.
+This block creates a parent `div` with a `wp-block-file` class name, which wraps a single `a` element. The Gutenberg editor allows file downloads to be displayed as buttons (`wp-block-file__button`) or text-only links. Our CSS styles both configurations as [buttons](/?path=/docs/components-button--elements).


House style question here (that might be for @tylersticka):

Do we prefer keeping Markdown paragraphs all on one line, or adding line breaks to avoid text extending past 80 characters?

Note that the text will render the same way, so this is strictly a code formatting question.

Personally, I'm a fan of semantic line breaks to keep code readable.

spaceninja · 2020-06-06T00:00:25Z

src/vendor/wordpress/_wordpress.scss

+   */
+
+  .wp-block-file__button,
+  :not(.wp-block-file__button) {


Could you change :not(.wp-block-file__button) to * and add the same comment explaining why here?

…atterns into chore/gutenberg-core-file

derekshirk · 2020-06-16T22:39:15Z

Currently blocked by #798

…atterns into chore/gutenberg-core-file

derekshirk · 2020-06-16T23:49:19Z

Now that #798 is merged, this PR is ready for re-review.

spaceninja · 2020-06-17T17:16:04Z

src/vendor/wordpress/_wordpress.scss

+  .wp-block-file__button,
+  * {


Can you add a comment somewhere explaining why you're targeting the class AND the universal selector? Earlier you had this comment, which was helpful to prevent someone else coming in later and thinking they can optimize your code by removing the class:

/* * If I don't include `wp-block-file__button` here, this selector is not * specific enough to enforce the desired styles, as rules from the * `@wordpress/block-library` override various properties. */

I don't think you need to repeat the comment in each selector — maybe add a footnote comment to each that references the earlier footnote?

Thanks for your feedback @spaceninja.

maybe add a footnote comment to each that references the earlier footnote?

Forgive my ignorance here, but I wasn't actually sure how to implement a "footnote comment" or a numerical comment, in the same way we do on properties of a classname.

So...this is what I ended up with:

/** * The following three selectors target a class name _and_ a universal * selector (*). This is intentional because the Gutenberg file block can * generate two different elements, a file-link and a file-button and we * want to style both of them with the same visual appearance and * characteristics. We could easily have chosen `:not(.wp-block-file__button)` * but opted for less characters. * * 1. Prevents default WP styling from modifying buttons opacity on hover. */ .wp-block-file__button, * { @include button.default; opacity: inherit !important; /* 1 */ } ...

derekshirk · 2020-06-18T16:28:59Z

@tylersticka At your leisure, would you like to review this before I merge?

tylersticka · 2020-06-18T16:44:54Z

@derekshirk I gave it a quick pass and it seems to do what I'd expect it to! I trust you and @spaceninja to have looked more deeply.

WIP applying button mixins

b661373

derekshirk self-assigned this Jun 3, 2020

tylersticka mentioned this pull request Jun 3, 2020

Style critical Gutenberg block markup #710

Closed

9 tasks

derekshirk added 3 commits June 3, 2020 10:56

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

c7b694f

…atterns into chore/gutenberg-core-file

styles gutenberg file blocks

1b5c3aa

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

98debdf

…atterns into chore/gutenberg-core-file

derekshirk added 3 commits June 3, 2020 16:37

adds developer comments

189eb31

updates story example and description

acceb1f

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

fe2ac14

…atterns into chore/gutenberg-core-file

derekshirk marked this pull request as ready for review June 3, 2020 23:42

derekshirk requested a review from a team June 3, 2020 23:42

adds changeset for minor release

33afc98

spaceninja reviewed Jun 5, 2020

View reviewed changes

derekshirk added 4 commits June 5, 2020 10:35

moves comment

ab867b0

simplifies scss

0b69218

adds additional markup examples to story

0103db7

adds semantic line breks to story description

43137f7

spaceninja reviewed Jun 6, 2020

View reviewed changes

derekshirk added 2 commits June 10, 2020 14:45

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

adce59a

…atterns into chore/gutenberg-core-file

modifies selector

35279c9

derekshirk added the ✋ blocked Can't proceed quite yet label Jun 16, 2020

Merge branch 'v-next' of https://github.com/cloudfour/cloudfour.com-p…

47f8c6f

…atterns into chore/gutenberg-core-file

derekshirk removed the ✋ blocked Can't proceed quite yet label Jun 16, 2020

simplifies CSS

f87ec99

derekshirk requested review from spaceninja and a team June 16, 2020 23:48

spaceninja reviewed Jun 17, 2020

View reviewed changes

updates sass doc comments

24e0515

derekshirk requested a review from spaceninja June 17, 2020 23:41

spaceninja approved these changes Jun 18, 2020

View reviewed changes

derekshirk requested a review from tylersticka June 18, 2020 16:12

derekshirk merged commit 62b6525 into v-next Jun 18, 2020

derekshirk deleted the chore/gutenberg-core-file branch June 18, 2020 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: style Gutenberg file block #773

Chore: style Gutenberg file block #773

derekshirk commented Jun 3, 2020 •

edited

Loading

changeset-bot bot commented Jun 3, 2020 •

edited

Loading

calebeby commented Jun 3, 2020

derekshirk commented Jun 4, 2020

spaceninja left a comment

spaceninja Jun 4, 2020

derekshirk Jun 5, 2020 •

edited

Loading

derekshirk Jun 5, 2020 •

edited

Loading

spaceninja Jun 5, 2020 •

edited

Loading

derekshirk Jun 6, 2020

spaceninja Jun 4, 2020

spaceninja Jun 5, 2020

spaceninja Jun 6, 2020

derekshirk commented Jun 16, 2020

derekshirk commented Jun 16, 2020

spaceninja Jun 17, 2020

spaceninja Jun 17, 2020

derekshirk Jun 17, 2020 •

edited

Loading

derekshirk commented Jun 18, 2020

tylersticka commented Jun 18, 2020

Chore: style Gutenberg file block #773

Chore: style Gutenberg file block #773

Conversation

derekshirk commented Jun 3, 2020 • edited Loading

Overview

Screenshots

Testing

Issue

changeset-bot bot commented Jun 3, 2020 • edited Loading

🦋 Changeset is good to go

calebeby commented Jun 3, 2020

derekshirk commented Jun 4, 2020

spaceninja left a comment

Choose a reason for hiding this comment

spaceninja Jun 4, 2020

Choose a reason for hiding this comment

derekshirk Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

derekshirk Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

spaceninja Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

derekshirk Jun 6, 2020

Choose a reason for hiding this comment

spaceninja Jun 4, 2020

Choose a reason for hiding this comment

spaceninja Jun 5, 2020

Choose a reason for hiding this comment

spaceninja Jun 6, 2020

Choose a reason for hiding this comment

derekshirk commented Jun 16, 2020

derekshirk commented Jun 16, 2020

spaceninja Jun 17, 2020

Choose a reason for hiding this comment

spaceninja Jun 17, 2020

Choose a reason for hiding this comment

derekshirk Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

derekshirk commented Jun 18, 2020

tylersticka commented Jun 18, 2020

derekshirk commented Jun 3, 2020 •

edited

Loading

changeset-bot bot commented Jun 3, 2020 •

edited

Loading

derekshirk Jun 5, 2020 •

edited

Loading

derekshirk Jun 5, 2020 •

edited

Loading

spaceninja Jun 5, 2020 •

edited

Loading

derekshirk Jun 17, 2020 •

edited

Loading