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

Social Links: Add ability to change social icon sizes #25921

Merged
merged 23 commits into from
Nov 6, 2020

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Oct 7, 2020

Description

This adds a size button control to the block toolbar, that lets you pick between 4 different sizes of icons (Small, Normal, Large, Huge). Those sizes affect the default style, the pill shape style, the logos only style, and even those variations of the new setup state as well.

Fixes #17276

How has this been tested?

  • (Backwards Compatibility) Create a post with social icons and confirm after applying the PR that they still can be edited and changed.
  • Add social icons, adjust their sizes using the sidebar
  • Confirm the new sizes look right in the editor and published.

Screenshots

social-link-size

Types of changes

  • Adds new attribute for size for Social Links
  • Adds CustomSelect control to adjust sizes
  • Adds custom CSS property for different sizes

@mkaz mkaz added the [Block] Social Affects the Social Block - used to display Social Media accounts label Oct 7, 2020
@mkaz mkaz requested review from nicolad and jasmussen and removed request for nicolad October 7, 2020 20:12
@github-actions
Copy link

github-actions bot commented Oct 7, 2020

Size Change: +206 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-library/editor-rtl.css 9 kB -53 B (0%)
build/block-library/editor.css 9 kB -56 B (0%)
build/block-library/index.js 147 kB +216 B (0%)
build/block-library/style-rtl.css 7.95 kB +49 B (0%)
build/block-library/style.css 7.95 kB +50 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 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 131 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.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.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.5 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 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.46 kB 0 B
build/edit-navigation/index.js 11.2 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.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/index.js 22.5 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 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.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 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 712 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.11 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.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 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 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 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.06 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

@jasmussen
Copy link
Contributor

This is what I see:

socl

Nice work! This will be a nice feature.

As you can see from my inspecting, it needs a little visual love that I can help with — more adaptive centering, to be specific, so as to work with arbitrary sizes.

One larger question, though, and I'll defer to folks like @nosolosw @kjellr @iamtakashi @alaczek and others: should this be a global CSS variable for all social links icons? Or should it be a per-block size setting? I've no real strong opinion but would love insights.

@oandregal
Copy link
Member

👋 Took a quick look at this and it seems that you may be able to go with preset sizes (like small/medium/large) for the icons at the moment, is that correct? If so, I'd suggest making this work with classes instead, as the font size control does for presets.

If you also wanted custom values provided by the user, that's a bit more entangled and there's no clear solution yet for this kind of setup (where we want to set a property with a custom value in the container that affects all children). The issue is kind of similar to this other PR #25372 The closest we've gone to address that use case is with the link control. The way it works there is by adding to the container a CSS variable with the custom value as an inline style. However, this approach is problematic with kses as it filters out any CSS variable within the style property of an element so it won't work for users without unfiltered_html permissions (author role, for example). See #25411 and Automattic/wp-calypso#45456. The path forward seems to be either teach kses to allow any CSS variable within inline styles or look for an alternative approach.

@kjellr
Copy link
Contributor

kjellr commented Oct 8, 2020

One larger question, though, and I'll defer to folks like @nosolosw @kjellr @iamtakashi @alaczek and others: should this be a global CSS variable for all social links icons?

I think a global size for all social links icons is adequate. As @nosolosw mentioned, I think a small/medium/large dropdown sort of setup (along with perhaps a custom value field) would be great.

@jasmussen
Copy link
Contributor

jasmussen commented Oct 9, 2020

A set of presets is a good idea as it allows us to pick presets that render the SVG icons well. Here are some thoughts:

Huge (48px/2x):

huge

Probably too huge, probably won't be used that often. But doesn't hurt to have it. Maybe you want to make a statement.

Large (36px/1.5x):

large

Big (32px/1.33_x) — best if we can do without this one as it's not a good scaling integer:

32 maybe

Default (24px/1x):

default

Small (18px/.75x):

small 18

Tiny (12px/.5x):

tiny 12

Perhaps too small, but it's a decent scaling integer

What do you think?

@kjellr
Copy link
Contributor

kjellr commented Oct 9, 2020

Those sizes look good! I agree we can probably lose the "Big" one. The "Tiny" one is likely too small to recommend to users, so I think it's fine to remove it from the presets.

@mkaz
Copy link
Member Author

mkaz commented Oct 13, 2020

@kjellr @jasmussen take a look, I implemented the drop down and sizes for Small, Normal, Large, and Huge.
These are the same names used in font sizing, so the set of 4 different sizes feels right.

Personally I think we could make everything a bit bigger, though it depends on if you are using filled or logo-only, play around with them and please feel free to adjust and edit the sizes if you want. We can also leave as is, they are implemented using CSS variables, so a theme could also overwrite them easily enough.

I need some help, the filled logos do not look proper in the editor, but do look fine when published.

social-link-sizes

@mkaz mkaz marked this pull request as ready for review October 13, 2020 17:53
@kjellr
Copy link
Contributor

kjellr commented Oct 14, 2020

This looks good! I think the sizes feel comfortable.

I need some help, the filled logos do not look proper in the editor, but do look fine when published.

I pushed a small change to fix that. In the editor, the button should be modified to fit the size of the icon.

Before:

Screen Shot 2020-10-14 at 11 58 31 AM
Screen Shot 2020-10-14 at 11 59 36 AM

After:

Screen Shot 2020-10-14 at 11 59 14 AM
Screen Shot 2020-10-14 at 11 59 23 AM

@mkaz mkaz changed the title Social Links: Add variables to change icon sizes Social Links: Add ability to change social icon sizes Oct 14, 2020
@jasmussen
Copy link
Contributor

This is what I see:

size

Nice work, this will make a bunch of people happy.

However a couple of things — this control doesn't really fit in the sidebar, in part because it's a primary action for the block. It should just be a popover that opens from the block toolbar, it can just be a text button like this:

Screenshot 2020-10-15 at 13 49 06

And the font size indication isn't really helping here, it seems fine to just do a menu like this:

Screenshot 2020-10-15 at 13 51 34

@jasmussen
Copy link
Contributor

Thanks so much for this PR, I know my feedback means a little extra work. But I think it will be worth it, and that doing it the right way will leave it without additional work necessary.

@mkaz
Copy link
Member Author

mkaz commented Oct 15, 2020

I've moved the controls to the toolbar, test it out and let me know what you think. 👍

@jasmussen
Copy link
Contributor

jasmussen commented Oct 16, 2020

Thank you, this is what I see:

size

This is definitely the right interaction, but the button doesn't look quite as fitting as I had hoped. I think we should still try it for now, but I may go back and change it to an icon separately, if I can find a good size icon.

There needs to be a checkmark on the right side of the selected size (normal being selected by default). There should be code for that in the more options dropdown:

Screenshot 2020-10-16 at 09 40 15

Also, because the popover menu extends from the toolbar, it needs to be of the dark toolbar material (black border, no shadow). I think there is an isPrimary prop on the popover component that makes this happen. It's used in the other menus:

Screenshot 2020-10-16 at 09 40 58

@jasmussen
Copy link
Contributor

This is unlikely to make it to 5.6, correct? Asking because looking at the code, I'd like to refactor it a bit, both to address Andrés feedback about not using CSS variables, but also to make the padding scale more naturally to the size. Ideally there's only one uniform padding that automatically pads more for larger icon sizes and less for smaller ones (like a percentage or something that scales with a value). But in order for that to work smoothly with both circles and pill shapes, I'll need to be able to focus on it for a bit, and I won't have time to do that until Monday.

@mkaz
Copy link
Member Author

mkaz commented Oct 16, 2020

@jasmussen I updated with the check icon and setting the isAlternate prop that gives the proper type of popup. Both of those changes should address what you had above.

I also rebased against the placeholder social links, but in the rebase I broke things and now have chubby icons. I figured you wanted to rework some of the CSS, and you'll end up doing it much better than me, I'll let you work your magic to get them right. Needed for both in editor and published view. Thanks!

social-links-size-update

@paaljoachim
Copy link
Contributor

My test shows this:

Social-icons-size

Btw
Should there also be a setting in the sidebar with a size drop down? Having it in the toolbar and the sidebar.

@jasmussen
Copy link
Contributor

I'm going to take a stab at bringing this home today!

Thanks for taking a look, Paal.

Should there also be a setting in the sidebar with a size drop down? Having it in the toolbar and the sidebar.

I'd suggest no: redundancy is okay in places, but it's hard to get right. It always begs the question: if there are two ways to accomplish the same, which one is the right way? And in absence of the redundancy, the principle to evaluate against is whether the action is primary (should be in toolbar) or secondary (should be in sidebar), and I'd suggest this is definitely primary.

@jasmussen
Copy link
Contributor

As I worked on this, and looking for a clear and scalable (for themes also) heuristic for what the padding is, I ended up with a basic system. 6x6 units. An icon is 4x4 units with 1unit of padding all around. So if an icon is 24px, then it has a 6px padding (24/6).

That works nicely with ems —font-size: 24px; width: 1em; height: 1em; padding: 0.25em; — however it did surface one gotcha.

18/4 = 4.5. And half-pixels don't render well, which means if we went with 18px for the "small" icon size, icons wouldn't be centered. They'd have 5px top and left padding, and 4px bottom and right padding.

So I changed the small size from 18 to 16. It looks alright:

Screenshot 2020-10-19 at 10 21 37

@jasmussen
Copy link
Contributor

@mkaz we need for the checkmark to be present on the "normal" size, even if you haven't explicitly set the property. I.e. insert a social links block, do nothing, check the size, and you see this:

need a default

That is — if you have no property explicitly set, the checkmark should also show on "normal".

mkaz and others added 23 commits November 4, 2020 11:46
This adds CSS custom properties to alter the circle/logo sizes. I'm not
quite sure how to integrate this in to make it adjustable, maybe as a
theme.json optoin?

First, we may want to create as fixed sizes (sm, med, large) to make
sure things look right. There are two values to adjust, the outer circle
and the inner logo. So it is possible to make the icons all out of sorts
if the sizes don't match properly.
Uses small, normal, large, and huge sizes matching the same presets as
the font sizes.

Uses the CustomSelectControl to pick which size

Introduces class names `has-SIZE-icon-size` that applies to social link
The default size is normal, even if the iconsize is not set,
so this sets the checkmark in the dropdown to reflect.
A test to see what we think of modifying the DropdownMenu/Button to
accept a text property which when specified and icon=null than it will
show instead of the icon.
Switches the button to display the name as the label for example "Large"
instead of just showing "Size"

Fix issue with { onClose } not being destructured

Pass toggleProps forward.
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.

One issue remains. Transforms are on two rows in this PR:

Screenshot 2020-11-06 at 16 05 23

However a rebase fixes that, and the issue is not present in the master branch:

Screenshot 2020-11-06 at 16 09 06

I suspect that an issue was created and fixed in the master branch, while this branch was ongoing.

But since rebasing will likely cause those beautiful green checks to fail a random amount of times, I'd say it's okay to merge this one if you first test whether you see the same issue as me, and that a rebase (even if not pushed) fixes it.

@mkaz mkaz merged commit 422b21c into master Nov 6, 2020
@mkaz mkaz deleted the add/17276-social-sizing branch November 6, 2020 15:38
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social Links Block: Add sizing options
10 participants