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 Block: Add icon and background color options #25372

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Sep 16, 2020

Description

Added options to the social links block allowing a user to specify the icon or background colors and have that apply to all the social links contained within it.

Note: Saving the styles in the social link blocks will be done if this general approach is continued instead of switching to the useColors hook if possible.

#21605

How has this been tested?

Only manual testing.
WordPress: 5.5.1

Testing Instructions

  1. Create a post adding social links block to it and adding urls for multiple platforms before saving.
  2. Apply this PR
  3. Reload the page, block should still be valid
  4. Select the social links block
  5. Select icon and background colors from the Inspector controls (include custom colors in testing)
  6. Save post and reload editor. Colors should be maintained.
  7. Switch style to logos only. The background color options should not be shown and when switching styles back the background color will have been cleared.

Screenshots

SocialIcons-Colors

Types of changes

New feature. Enhancement.

Next Steps

  • Add class names and styles to social link saving for front end display
  • Switch to using __experimentalUseColors if its possible to achieve same editor functionality. (didn't work for me)
  • Refine approach

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review September 21, 2020 00:53
@aaronrobertshaw aaronrobertshaw changed the title [WIP] Social Links Block: Add icon and background color options Social Links Block: Add icon and background color options Sep 21, 2020
@aaronrobertshaw
Copy link
Contributor Author

@apeatling and @glendaviesnz these color options are working for me although I feel there should be a better way of achieving it than I've come up with. I'd appreciate any suggestions or feedback you have.

packages/block-library/src/social-links/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/social-link/edit.js Outdated Show resolved Hide resolved
@apeatling
Copy link
Contributor

This functions well. I think there should be a contrast-checker component in place for the checking the background and icon color combination selected.

@aaronrobertshaw
Copy link
Contributor Author

I've added a contrast checker although it doesn't really handle the default backgrounds that vary in color by the social media platform.

This PR has also been rebased and conflicts resolved after the addition of the "open in new tab" feature for social links. I also had to change the approach a little as for the moment colors cannot be retrieved from the general settings ( #25419 ). There is a PR to re-add the default colors and gradients ( #25523 ) however updating this to leverage the __experimentalUseEditorFeature approach might be better long term.

$url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
$label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : block_core_social_link_get_name( $service );
$class_name = isset( $attributes['className'] ) ? ' ' . $attributes['className'] : false;
$service = ( isset( $attributes['service'] ) ) ? $attributes['service'] : 'Icon';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this and the others could be updated to use array_key_exists( 'service', $attributes ). That seems to be a cleaner way that some of the other core blocks now use.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Sep 23, 2020

This all seems to work as advertised.

Apologies if you already explored this, but the specificity of the individual link color settings is reasonably light, so rather than having to pass down context and set individual attributes for each embedded link is there some way we could instead add a generic style at the parent level? eg. adding a style of:

.override-background.wp-block-social-links:not(.is-style-logos-only) .wp-social-link {
    background-color: red;
}

replaces the background on all the child icons.

@aaronrobertshaw
Copy link
Contributor Author

Apologies if you already explored this, but the specificity of the individual link color settings is reasonably light, so rather than having to pass down context and set individual attributes for each embedded link is there some way we could instead add a generic style at the parent level?

That was my thinking initially as well, to be able to just apply the color choices to the social links block itself and allow the individual links to inherit that as required. Unfortunately, I couldn't find a way to achieve the desired results doing this.

Some of the issues I ran into in this area were:

  • Maintaining consistency by allowing the normal use of color CSS classes e.g. has-accent-background-color
  • Not being able to use those color classes on the container without them being applied but still inheritable (e.g. background colors).
  • As the color palette could be defined by the theme, I couldn't create a new set of styles setting the correct color like the example you gave
  • Custom colors in general using inline styles make the background colors on the parent a problem again.
  • Still maintaining the individual social media service colors without blowing out the CSS footprint

I could be missing something so if you do have further suggestions, let me know and I can explore those. For now though, this does give us the control over colors we need, hopefully that is ok.

@glendaviesnz
Copy link
Contributor

I could be missing something so if you do have further suggestions, let me know and I can explore those. For now though, this does give us the control over colors we need, hopefully that is ok.

No, I don't think you are missing anything, it just seemed like there should be a simpler approach, but due to the factors you list I think the approach you have take is the only way to push the styles down to the children.

@glendaviesnz
Copy link
Contributor

@apeatling do you have any suggestions on who is best to give us a signoff on this approach?

@apeatling
Copy link
Contributor

Maybe @nosolosw or @youknowriad might have thoughts on the approach?

@youknowriad
Copy link
Contributor

Hi there! thanks for the PR.

In general, we're trying to move away from specific customization and attributes in favor of generic support flags that can be enabled across blocks, so my questions here are:

Can we use the __experimentalColor support flag for the background color support here instead? It does support text and backgrouund color and it seems the link color can be considered as the same thing as "icon color"? Can't we just provide an option to change the "label"?

@aaronrobertshaw
Copy link
Contributor Author

Hi @youknowriad, thanks for taking a look at this!

I'm still trying to get my head around how everything fits together. I did initially try to leverage __experimentalColor however I ran into problems applying the colors only to each child within the Social Links' InnerBlocks.

Looking back at this PR, do you think an option would be to use __experimentalColors without wrapping the InnerBlocks with the generated color components. Then still make those color attributes available to the inner links via the block context?

@youknowriad
Copy link
Contributor

youknowriad commented Sep 25, 2020

I was thinking __experimentalColors could be applied to SocialLink and not SocialLinks? or is this not good enough in terms of UX? It's how the "buttons + button" block works.

@aaronrobertshaw
Copy link
Contributor Author

The idea was, the better UX was for the user to only have to make a single set of color selections. That is the approach used by the navigation block.

@youknowriad
Copy link
Contributor

I wonder if there's a way to make the settings on the parent component work as a "default value" for its children instead.

@oandregal
Copy link
Member

@youknowriad I don't have an answer for this at the moment but logged this example at #22887 because I smell that these little glitches in the system may be addressed together.

@aaronrobertshaw
Copy link
Contributor Author

@youknowriad and @nosolosw would you be happy for us to move forward with this current approach so we have the functionality in place? I'm happy to come back and revisit this when we have a solution to the larger questions raised.

@youknowriad
Copy link
Contributor

Personally, I'd rather compromise on the UI and make this per icon for now until we figure out a better API for handling parent -> child attributes UI. What do you think here @mtias

@mtias
Copy link
Member

mtias commented Oct 1, 2020

It doesn't make sense per icon to me unless it's just an implementation detail (the control lives int he wrapper and propagates)

@aaronrobertshaw
Copy link
Contributor Author

@apeatling Would you like to weigh in and cast a vote on a preferred approach to this one?

Should we allow what is the best experience for a user, to be the tie breaker if needed?

@apeatling
Copy link
Contributor

I agree that changing per icon is not the ideal user experience, and we're better off optimizing for the user.

@skorasaurus skorasaurus added the [Block] Social Affects the Social Block - used to display Social Media accounts label Dec 6, 2020
@aaronrobertshaw
Copy link
Contributor Author

An alternative approach to this is available in #28084

I'm inclined to close this PR in favour of the new approach if others agree.

@aaronrobertshaw
Copy link
Contributor Author

Closing this PR in favour of #28084

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.

7 participants