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

Update 'Quote' pattern #23881

Merged
merged 3 commits into from
Jul 17, 2020
Merged

Update 'Quote' pattern #23881

merged 3 commits into from
Jul 17, 2020

Conversation

enriquesanchez
Copy link
Contributor

Updates the 'Quote' pattern, adds an image on top and a separator at the bottom.

Before:

Screen Shot 2020-07-10 at 12 34 37

After:

Screen Shot 2020-07-10 at 12 34 15

@enriquesanchez enriquesanchez added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Jul 10, 2020
@enriquesanchez enriquesanchez self-assigned this Jul 10, 2020
@github-actions
Copy link

github-actions bot commented Jul 10, 2020

Size Change: -1.02 MB (88%) 🏆

Total Size: 1.15 MB

Filename Size Change
build/a11y/index.js 0 B -1.14 kB (0%)
build/annotations/index.js 0 B -3.67 kB (0%)
build/api-fetch/index.js 0 B -3.39 kB (0%)
build/autop/index.js 0 B -2.82 kB (0%)
build/blob/index.js 0 B -620 B (0%)
build/block-directory/index.js 0 B -7.67 kB (0%)
build/block-editor/index.js 0 B -115 kB (0%)
build/block-editor/style-rtl.css 10.8 kB +29 B (0%)
build/block-editor/style.css 10.8 kB +28 B (0%)
build/block-library/editor-rtl.css 7.58 kB -19 B (0%)
build/block-library/editor.css 7.57 kB -19 B (0%)
build/block-library/index.js 0 B -132 kB (0%)
build/block-serialization-default-parser/index.js 0 B -1.88 kB (0%)
build/block-serialization-spec-parser/index.js 0 B -3.1 kB (0%)
build/blocks/index.js 0 B -48.2 kB (0%)
build/components/index.js 0 B -199 kB (0%)
build/components/style-rtl.css 15.6 kB -232 B (1%)
build/components/style.css 15.6 kB -227 B (1%)
build/compose/index.js 0 B -9.67 kB (0%)
build/core-data/index.js 0 B -11.4 kB (0%)
build/data-controls/index.js 0 B -1.29 kB (0%)
build/data/index.js 0 B -8.46 kB (0%)
build/date/index.js 0 B -5.38 kB (0%)
build/deprecated/index.js 0 B -772 B (0%)
build/dom-ready/index.js 0 B -569 B (0%)
build/dom/index.js 0 B -3.23 kB (0%)
build/edit-navigation/index.js 0 B -10.8 kB (0%)
build/edit-post/index.js 0 B -304 kB (0%)
build/edit-post/style-rtl.css 5.61 kB +20 B (0%)
build/edit-post/style.css 5.61 kB +20 B (0%)
build/edit-site/index.js 0 B -16.6 kB (0%)
build/edit-site/style-rtl.css 3.06 kB +32 B (1%)
build/edit-site/style.css 3.06 kB +34 B (1%)
build/edit-widgets/index.js 0 B -9.35 kB (0%)
build/editor/index.js 0 B -45.1 kB (0%)
build/element/index.js 0 B -4.65 kB (0%)
build/escape-html/index.js 0 B -733 B (0%)
build/format-library/index.js 0 B -7.72 kB (0%)
build/hooks/index.js 0 B -2.13 kB (0%)
build/html-entities/index.js 0 B -622 B (0%)
build/i18n/index.js 0 B -3.56 kB (0%)
build/is-shallow-equal/index.js 0 B -709 B (0%)
build/keyboard-shortcuts/index.js 0 B -2.51 kB (0%)
build/keycodes/index.js 0 B -1.94 kB (0%)
build/list-reusable-blocks/index.js 0 B -3.12 kB (0%)
build/media-utils/index.js 0 B -5.32 kB (0%)
build/notices/index.js 0 B -1.79 kB (0%)
build/nux/index.js 0 B -3.4 kB (0%)
build/plugins/index.js 0 B -2.56 kB (0%)
build/primitives/index.js 0 B -1.4 kB (0%)
build/priority-queue/index.js 0 B -789 B (0%)
build/redux-routine/index.js 0 B -2.85 kB (0%)
build/rich-text/index.js 0 B -13.9 kB (0%)
build/server-side-render/index.js 0 B -2.71 kB (0%)
build/shortcode/index.js 0 B -1.7 kB (0%)
build/token-list/index.js 0 B -1.27 kB (0%)
build/url/index.js 0 B -4.06 kB (0%)
build/viewport/index.js 0 B -1.85 kB (0%)
build/warning/index.js 0 B -1.13 kB (0%)
build/wordcount/index.js 0 B -1.17 kB (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.min.js 124 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.77 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/index.min.js 200 kB 0 B
build/compose/index.min.js 9.67 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-site/index.min.js 16.8 kB 0 B
build/edit-widgets/index.min.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.1 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 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.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 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.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.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.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 13, 2020
@kjellr
Copy link
Contributor

kjellr commented Jul 13, 2020

This is cool! Where does the user icon come from? It seems a little inelegant, and I wonder if there's something else we can use.

@enriquesanchez
Copy link
Contributor Author

This is cool! Where does the user icon come from?

@kjellr I'm pulling it from Gravatar.

I wonder if there's something else we can use.

Would you suggest a variation of that same idea? Or something totally different?

@kjellr
Copy link
Contributor

kjellr commented Jul 13, 2020

It would be sort of cool to pull an image of Herman Melville if there is a CC0 one (but I know that might open a can of worms around images we can use here). Alternatively, maybe if there's a G2-style icon we can use instead of this one?

@enriquesanchez
Copy link
Contributor Author

maybe if there's a G2-style icon we can use instead of this one?

@kjellr Here are a couple of similar graphics using the user icon from G2:

Screen Shot 2020-07-13 at 16 31 21

Screen Shot 2020-07-13 at 16 34 11

@kjellr
Copy link
Contributor

kjellr commented Jul 14, 2020

I was thinking maybe something like this? I matched the background to the default text color, which makes this a little more bold, too.

Frame 3

(Here's the SVG I used there)

@enriquesanchez
Copy link
Contributor Author

That looks great. Will add this graphic to the request already in progress in Trac https://core.trac.wordpress.org/ticket/50650

@enriquesanchez
Copy link
Contributor Author

Updated the avatar image with the new one from the wp.org CDN:

Screen Shot 2020-07-16 at 18 59 57

@kjellr @mapk Would love one last review from you.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@ZebulanStanphill
Copy link
Member

Something I'm not certain about is whether or not it is semantically correct to use an hr element here. Perhaps that should be removed? It looks like it's only being used for visual effect here, which is not how semantic elements should be used.

@ZebulanStanphill ZebulanStanphill added Needs Accessibility Feedback Need input from accessibility and removed Needs Accessibility Feedback Need input from accessibility labels Jul 17, 2020
@joedolson
Copy link
Contributor

As a general rule, since the visual use of an HR is the same as its semantic usage, it's semantically correct. There obviously can be cases where a separator isn't really a separator, but usually it's fine.

It's fine for there to be an HR used as a separator; that's not a problem. If it's changeable, then it's really not a big deal, as a user can remove it if they don't want a separation.

The HR represents a separation; if the user wants a visual separation, then that visual separation should also be represented in the page semantics.

I'd actually consider a horizontal rule that creates a visual separation that isn't reflected semantically to be a bigger problem.

We can't dictate semantics to a user, because this block pattern doesn't exist within a context; to judge whether an HR is appropriate or not would be dependent on what other content surrounds the content, and isn't inherent to the HR itself.

@ZebulanStanphill
Copy link
Member

(Full discussion in the Slack accessibility channel.)

Seeing that this is probably fine (and if not, the potential negative impact is minimal), I'm going to go ahead and merge this. It's simple enough to change later if desired.

@ZebulanStanphill ZebulanStanphill merged commit 1488250 into master Jul 17, 2020
@ZebulanStanphill ZebulanStanphill deleted the update-quote-pattern branch July 17, 2020 17:46
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 17, 2020
@ellatrix ellatrix mentioned this pull request Jul 20, 2020
6 tasks
@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants