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

Block: social links. #16897

Merged
merged 54 commits into from
Sep 6, 2019
Merged

Block: social links. #16897

merged 54 commits into from
Sep 6, 2019

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Aug 3, 2019

Description

Fixes: #1873

References

slack discussion
Prototype

How has this been tested?

  • Add new Social Links block
  • Confirm you can insert multiple links
  • Icons and links show on view
  • Can create and use block as Author permission

Screenshots

(see updates in thread)

Types of changes

Adds a new block for inserting social media links. The block will try to match the proper icon to the URL typed in, or the user can select the icon by clicking and selecting from a list.

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.

@nicolad nicolad added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress New Block Suggestion for a new block labels Aug 3, 2019
@nicolad nicolad requested a review from mkaz August 3, 2019 14:27
@nicolad nicolad self-assigned this Aug 3, 2019
@nicolad
Copy link
Member Author

nicolad commented Aug 3, 2019

Hello @mkaz, thank you for your comments on the previous PR.
I opened this PR to make it possible to combine our efforts. Please let me know what is the branch name you are working on, so I can improve this block based on that and then have a better starting point.

@nicolad nicolad mentioned this pull request Aug 3, 2019
5 tasks
@mkaz
Copy link
Member

mkaz commented Aug 5, 2019

I made a fork off your old code base and created a PR to show the difference.
https://github.com/nicolad/gutenberg/pull/1

If you like I can update and push that to this branch, I wanted you to review before I combined. It is still a work in progress, we need some clarification on what icon set to use.

I like your idea of filling in the icon automatically based on the URL. I think we might be able to do both, if no icon is picked and the user types twitter.com... than the twitter icon shows; but also allow the user to click and set an icon if not found or they want to change.

@nicolad
Copy link
Member Author

nicolad commented Aug 5, 2019

I made a fork off your old code base and created a PR to show the difference.
nicolad#1

If you like I can update and push that to this branch, I wanted you to review before I combined. It is still a work in progress, we need some clarification on what icon set to use.

I like your idea of filling in the icon automatically based on the URL. I think we might be able to do both, if no icon is picked and the user types twitter.com... than the twitter icon shows; but also allow the user to click and set an icon if not found or they want to change.

Thank you for the help @mkaz, I will check and apply those changes to the current PR.

@mkaz
Copy link
Member

mkaz commented Aug 5, 2019

Also, I was just pointed to our set of social icons we can use here:
https://github.com/Automattic/social-logos

@nicolad
Copy link
Member Author

nicolad commented Aug 5, 2019

Also, I was just pointed to our set of social icons we can use here:
https://github.com/Automattic/social-logos

I saw that in the prototype we have 2 styles, but social-logos don't have that option available(at least I didn't saw this being exposed via props).
image

@mkaz
Copy link
Member

mkaz commented Aug 5, 2019

@nicolad Yeh, I saw that. Styles is an extra piece I think we can add on, once we get the link builder and picker in place and working well. I think we can add additional enhancements, also choosing color or matching theme, circles, etc...

@mkaz
Copy link
Member

mkaz commented Aug 7, 2019

I just pushed up a set of changes which should be workable. There are still a lot of polish and refinements to be made to the block, but general UI should be in a testable set.

@mkaz
Copy link
Member

mkaz commented Aug 12, 2019

I think this is at a pretty good point for a design review. The interaction with InnerBlocks and hiding and showing URL fiels based on selection is still not quite correct, but I'm not sure what direction to go, since there are two fields icon/url that can be set per link.

@mapk, @melchoyce, and/or @karmatosed you all commented on the previous PR and issue. So if you are still interested, or any other designer input is welcome.

@mkaz mkaz added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository and removed First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels Aug 12, 2019
@gziolo
Copy link
Member

gziolo commented Aug 13, 2019

@matias, @youknowriad and @mapk - how much individual Social Link block and Logo block (#16484) differ conceptually? I guess it's fine to proceed as two independent items, but this is another case where we should be thinking about having a block which depending on context could behave slightly different. I have no idea yet how this could be materialized but definitely something we should thing about :)

packages/block-library/src/index.js Outdated Show resolved Hide resolved
const SocialLinkEdit = ( { attributes, setAttributes, isSelected } ) => {
const { icon, url } = attributes;

const classes = classNames( 'wp-social-icon', `wp-social-icon-${ icon }` );
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we want to maintain on WordPress core scope from now on? At the moment corresponding styles will be loaded only together with the block.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure I understand this, this is how I would implement in a plugin, not quite sure how we should for core. Open to any suggestions

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

@gziolo Any chance I could get you to take a screen capture of the block for reference?

@jasmussen jasmussen requested review from oandregal and a team September 4, 2019 18:06
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Code-wise this is 👍

@mkaz mkaz merged commit f4406ed into master Sep 6, 2019
@mkaz mkaz deleted the block/social-links branch September 6, 2019 17:31
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
event.preventDefault();
setPopover( false );
} } >
<div className="editor-url-input block-editor-url-input">
Copy link
Member

Choose a reason for hiding this comment

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

Class names are not meant to be reused outside the component which defines them. In this case, we should go through the abstraction of the URLInput component, not use its classes directly. Or, alternatively, copy all of its styles to a class specific to the Social Link. These guidelines are meant to ensure that styles only apply within the context of a component. Otherwise, we risk future breakage on changes expected only to affect URLInput.

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#naming

A component's class name should never be used outside its own folder (with rare exceptions such as _z-index.scss). If you need to inherit styles of another component in your own components, you should render an instance of that other component. At worst, you should duplicate the styles within your own component's stylesheet. This is intended to improve maintainability by treating individual components as the isolated abstract interface.

jasmussen pushed a commit that referenced this pull request Dec 4, 2019
Address feedback in #16897 (comment).

Something's missing, though, I'm getting `Cannot read property 'value' of undefined`. Could use advice here.
mkaz pushed a commit that referenced this pull request Dec 5, 2019
* Social Links: Use URLInput

Address feedback in #16897 (comment).

Something's missing, though, I'm getting `Cannot read property 'value' of undefined`. Could use advice here.

* Docs: Clarify intention of common components

* Block Library: Use next value from URLInput onChange callback
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
@aduth
Copy link
Member

aduth commented Feb 26, 2020

Was it intentional or discussed anywhere that this block be implemented as a dynamic block, as opposed to strictly a static block with a save function to render its saved output? It doesn't seem to me that this block needs to be generated dynamically, as the server-side implementation appears to me to be generating only static content.

Edit: On further evaluation, I can see how it might make sense that this be considered dynamic if there's the possibility that a provider may change its logo at some point in the future.

@oandregal
Copy link
Member

@aduth the reason is that lower-privilege users aren't allowed to add SVG to post_content.

@aduth
Copy link
Member

aduth commented Feb 26, 2020

@nosolosw Thanks, I can see how that might be problematic. But at least under that line of thinking, I would still consider it to be static content, ideally not a dynamic block. I might wonder if it's really a security issue to allow a contributor to publish content with svg (i.e. maybe we should loosen that restriction).

I did also edit my original comment in the meantime, since I do think there could be argument made that these are technically dynamic content, if you consider that a social media provider may choose to update their logo at some point in the future, and by implementing this as a dynamic block, we can easily update all instances of that block in previously-published content.

@mcsf
Copy link
Contributor

mcsf commented Feb 26, 2020

there could be argument made that these are technically dynamic content, if you consider that a social media provider may choose to update their logo at some point in the future, and by implementing this as a dynamic block, we can easily update all instances of that block in previously-published content.

More or less rehashing arguments from Slack: technically, the essential content of the block is the reference to the social media provider (e.g. service: 'spotify'); the actual logo is derived data that can and should be pushed out of the static blocks (and, say, moved to stylesheets, which are subject to change).

@mkaz
Copy link
Member

mkaz commented Feb 26, 2020

I do think allowing the SVG content would be the best possible solution. However, here's a good presentation explaining the dangers of SVG and why the content can't be trusted: https://www.owasp.org/images/0/03/Mario_Heiderich_OWASP_Sweden_The_image_that_called_me.pdf

My thought would be to limit the block to Admins only, and restrict it from showing up to users without the privilege. This would be a nice feature regardless of this block. #20177

I do have a PR here that moves the icons out to CSS, this would allow us to make it not dynamic #19888

@mtias
Copy link
Member

mtias commented Feb 26, 2020

Main reason was avoiding serializing SVG markup in the source, which would make any updates to logos unfeasible. The better solution would be to generate the SVGs through CSS so that markup can still be static while the icons can be iterated by updating the stylesheet. The support for coloring and such wasn't great at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks New Block Suggestion for a new block [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Block: Social Links