Social Icons: Add examples for Social Link variations. #62767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndiego
added
[Type] Enhancement
|
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. |
|
Size Change: +19 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
talldan
reviewed
Jun 24, 2024
Comment on lines
+353
to
+358
| variation.example = { | ||
| attributes: { | ||
| service: variation.name, | ||
| url: '#', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I think adding a viewportWidth in the example object would be good as the preview is quite small and scaled down.
A value of 0 results in no scaling which is an option. 150 results in the icon nicely centered but zoomed in and showing some pixelation.
talldan
reviewed
Jun 24, 2024
There was a problem hiding this comment.
It might be ok to ship this as it is, but the trouble is the preview isn't truly showing what the social icon will look like. For example, the WordPress icon is supposed to look like this:
But the preview is showing this:
This might be why there's no example 😄
I think this is a side effect of how the block has been implemented. The example is showing a social link icon without the social links parent, but the social links block is what provides all the styles at the moment. I have no idea why it was implemented like this, my feeling is that each block should be responsible for providing its own base styles.
A fix would be to refactor the css of the blocks, though lots of care would have to be taken to make sure nothing is broken along the way.
t-hamano
reviewed
Jun 24, 2024
There was a problem hiding this comment.
Another concern is that it generates invalid HTML in the preview area. The li elements need to be placed as children of ul or ol element, but the parent of the li element is a div.
If the block itself could know in what context it was being rendered, it might be possible to, for example, wrap it in a Social Links block or remove the li element, but as far as I know there is no way to do this 🤔
|
That's a good observation. Maybe supporting a example: {
name: 'core/social-links',
innerBlocks: [
{
name: 'core/social-link',
attributes: { service: variation.name, url: '#' },
},
],
} |
I think this is a good idea. We might be able to ship this PR before that approach is implemented, but I'm hesitant about whether to tolerate invalid markup. |
One drawback I can think of is that if a user selects a different social links style (e.g. 'Huge' size, with 'Pill Shaped' block style), then the preview would show the incorrect style, and it might be quite hard to fix that with the way the social links block is hard coded. Having said that, it already seems to be a problem with other blocks (e.g. if you trying adding a paragraph inside a quote, the preview shows the paragraph with the wrong style). edit: It could be an option like |
|
Well, at the very least, I am glad this PR started a good discussion around block previews. 😉 The "No preview available" message feels like something is broken, especially when it appears on Core blocks, and this situation with child blocks is relatively common. I have a block that uses a similar approach. I think adding the
I am not sure the example needs to represent the current state of the block when inserted in a specific spot or styled in a specific way by the parent. That sounds like it would get complicated very quickly. As long as it displays a realistic default state for the block, it's a good baseline IMO. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
The variations for the Social Link block do not have the
exampleproperty set, which causes the block inserter to display the messageNo preview available. This PR fixes that.How?
The following code to adds example content to each variation. I've set
#for theurlproperty since unlinked icons have opacity applied.Testing Instructions
Screenshots or screencast
The preview is not the best, but it's better than displaying no preview at all.