-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Icon: Try auto-matching variation based on URL #59303
base: trunk
Are you sure you want to change the base?
Conversation
const classes = classNames( 'wp-social-link', 'wp-social-link-' + service, { | ||
'wp-social-link__is-incomplete': ! url, | ||
[ `has-${ iconColor }-color` ]: iconColor, | ||
[ `has-${ iconBackgroundColor }-background-color` ]: | ||
iconBackgroundColor, | ||
} ); | ||
|
||
const [ showPopover, setShowPopover ] = useState( ! url && ! service ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inital state change allows Popover to be opened for freshly inserted blocks.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
} | ||
|
||
setAttributes( nextAttributes ); | ||
} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should provide a hit below the URL input for this new feature. Not sure about the design or text thought 😅
Size Change: +722 B (+0.04%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
Flaky tests detected in 0014fde. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10598789915
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this and works as advertised :)
Feel free to wait for more technical / design feedback but from my perspective this is a great enhancement as is and I don't see any issues with the code :)
Thank you, @fabiankaegy! What do you think about services that can be matched via URL? Do you think inserting them via the global inserter is a good option? |
Hmmmm good point. I missed that. I had assumed (not tested well enough) that clicking the plus icon would still bring up the block picker. How do you feed about solving this by adding It looks odd right now because it doesn't use the dropdown but instead shows all the icons which I think is way too much. But kind of nice to have the ability to switch manually |
I think it would make sense to adjust this conditional here: gutenberg/packages/block-editor/src/components/block-variation-transforms/index.js Line 192 in 599f336
|
Currently, transformations in the Inspector panel don't scale well. @ntsekouras, is working to improve that in #46726. Once that PR is merged, we can add the `scope' to the variations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working better than expected. Before you merge, I'd love a quick gut-check from @richtabor as well, but from my end, this works great:
That said, it seems nice to potentially explore a separate improvement to also auto-complete the https://
part of the URL. I.e. I'd love to be able to type in codepen.i
and as soon as I type in the final o
, it updates the whole to https://codepen.io
.
Also separately, I feel like this PR potentially unlocks the Social Icon block to work on its own, without the Social Icons container:
I'd love to be able to insert just that one block and have it transform based on the URL. It does not need to deprecate the container block, and we'd want to copy over the color/style controls to work on the block itself. But it would be a huge win for things like adding social links in a navigation block or elsewhere.
That's quite interesting. |
I don't think this would work, as we're introducing the concept that a link relates to a social icon, but if we open up transforms, then we're breaking that relationship — as you can transform to a Facebook block, but have a Twitter link. We could clear the link when transforming, but then again, with this new relationship, it's not a Facebook icon without a Facebook link. |
Definitely doable.I am happy to follow up on this one.
100%. A hint or help text below the input would be nice, and I would appreciate your help. |
LinkControl has a |
This is looking great, but I still would want to be able to add a Facebook icon with a dummy link for testing purposes. You also see this in patterns fairly often where the URL is set to |
@ndiego, that was also brought up in the issue . You should be able to use |
I believe that was the reason @ntsekouras closed PR, which was exploring enabling transformations for the Social Icons.
|
It's noble in principle to seek a correct URL, but we should also not jump through hoops or hold off meaningful improvements because it's possible to create a situation that isn't ideal. This will always be possible in editing software, no matter how much you do, at worst you could edit the markup manually. I like to think of us providing useful guardrails and smart defaults instead. Especially in the mastodon case, we have to open up those guardrails because the URL will often differ. |
Valid point, @jasmussen! I'll try to finalize feature early next week. |
241287b
to
5ad0ed4
Compare
5ad0ed4
to
1586c6b
Compare
@Mamaduka reached out to me on WP Slack and he's happy for me to take this PR forward towards merge. @jeryj @MaggieCabrera @ajlende If you're able to help with testing and reviews that would be awesome 🙏 |
import { getProtocol, isValidProtocol } from '@wordpress/url'; | ||
|
||
// Please see packages/block-editor/src/components/link-control/is-url-like.js | ||
export default function isURLLike( val ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to ship this PR, I'm going to raise a PR to get this moved into the @wordpress/url
package. Then I'll reuse it in both here and LinkControl
.
0014fde
to
4b67c7d
Compare
My only remaining concern here is that we've effectively made it harder to add specific icons. Unless you type in the URL of the service correctly it's now pretty hard to insert a specific social icon. I'd say we should consider something akin to the way the However, this would require moving the code to use I'd be curious to hear whether folks agree (cc @richtabor @jasmussen). |
Adding social links is already so difficult and complex a process due to how the block is built, that with this PR the user experience will be a net win, even if adding obscure icons may be slightly less obvious. If we're concerned, Aki's suggestion of a "Replace" dropdown in the block toolbar for each item, would be a good addition. |
Hopefully this has been somewhat improved in #64877
Yes I agree it is an improvement. ...even if adding obscure icons may be slightly less obvious. My primary concern would be that some people don't think about URLs like we do. Instead they think "I want to add a Facebook icon". So when they are faced with an form input they will be unsure about how to get the Facebook icon. Moreover, they won't get the Facebook icon unless they type I'm suggesting we consider providing a UI which also provides a means to
Something like this... Screen.Capture.on.2024-08-30.at.12-48-21.1.mp4I appreciate it's a little rough in this implementation, but it's a pattern we use elsewhere (Navigation block). Note: the code for the above demo is not on this PR.
That might be a good stop gap, but for me it's too hidden to offer a long term solution to the problem I'm attempting to illustrate above. To be clear, I don't want to block this PR, but I do want to flag the alternative user flow that I'm concerned about. We had a similar issue with the Nav block where we over optimised for a particular linear flow which then caused knock on confusion for another set of users who expected a different flow. |
There could be something to it. I'm wary of adding too much complexity to the URL component, but effectively you could think of the 2nd level icon selector as an auto-complete preview, and explicit choice. |
I agree with this. In my mind, the flow is: add the icon I want, then set the link. Another flow I use personally, especially when building patterns, is inserting the most common icons and setting the link to |
Once the service is set, it's set. So with the UI I'm suggesting above folks could still use the flow you are outlining. |
What?
Resolves #56259.
PR updates Social Icon's block to auto-match service variation based on the URL pattern. It also enables direct insertion of the block.
Note
Some variations like Mastodon, which allow services to be hosted on different domains, can't be auto-matched. But they can be inserted using the global block inserter.
Co-authored-by: Dave Smith [email protected]
Why?
See #56259.
How?
Define pattern matches for each variation, similar to how Embeds block implementation.
Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast
CleanShot.2024-02-23.at.09.40.34.mp4