-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RNMobile] Social icons #23017
[RNMobile] Social icons #23017
Conversation
Size Change: +19 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
b091d24
to
a0edba7
Compare
b7a46c7
to
631daee
Compare
packages/block-editor/src/components/block-list/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/index.native.js
Outdated
Show resolved
Hide resolved
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.
LGTM! Nice work @lukewalczak! 👏 Tested it on the demo app (iOS/Android) and also on WordPress iOS.
@@ -434,7 +434,8 @@ export function createBlockWithFallback( blockNode ) { | |||
if ( name && name.indexOf( 'core/social-link-' ) === 0 ) { | |||
// Capture `social-link-wordpress` into `{"service":"wordpress"}` | |||
attributes.service = name.substring( 17 ); | |||
name = 'core/social-link'; | |||
// Display social link service name for mobile platform | |||
name = Platform.OS === 'web' ? 'core/social-link' : name; |
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.
Can you help me understand why the deprecated name needs to be kept in mobile?
My worries:
- Even though we should keep this compat shim for years to preserve older content, the idea is still to help move content forward by remapping block names, so that, when a user edits an old post, the Social Link markup is correctly updated.
- The shim is likely to move out of the parser and be placed somewhere more sustainable (e.g. external compat file). This could be provided by JS filters or some other form of hooking. Once we do that, it would be important that the shim unconditionally reassigns
name
tocore/social-link
.
/cc @lukewalczak @hypest
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.
Hey @mcsf, that change was basically needed to display specific social icon name and its icon within toolbar on the mobile platform.
Without this change, it would be confusing as each social-icon
would have the same label and the same icon after saving the post.
without parser change | with parser change |
---|---|
The introduced change was the easiest solution to fulfil the requirement, but I understand your concerns and will double-check if we can achieve the same effect without parsers changes.
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.
However, could you please elaborate more on:
The shim is likely to move out of the parser and be placed somewhere more sustainable (e.g. external compat file). This could be provided by JS filters or some other form of hooking. Once we do that, it would be important that the shim unconditionally reassigns name to core/social-link
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.
Thanks for the context.
It's clear then that support for Block Variations needs to be improved so that those interactions can take into account attributes like a variation's title or icon.
For comparison, even on the Web we let the variation show in certain places and let the base type show in others, which is also a problem but less confusing than in the apps:
See open questions about variations: #16283 (comment)
However, could you please elaborate more on:
The shim is likely to move out of the parser […]
Sure. I'm referring to this comment by Grzegorz.
Context:
- Block variations API: Variations (formerly patterns) API for blocks #16283, Blocks: Match blocks in the inserter using keywords from patterns #19243
- Social Icons implementation: Social Link: group all variations under one block type #19887
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.
Correct me if I'm wrong: I assume that the main goal is to revert that change and have equal HTML on both platforms?
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.
Yep, correct.
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.
I've created a PR fixing that, please check it ✌️
Description
NOTE: That PR should be merged after #23299 since it's using theLinkSettings
component coming from there.NOTE: That PR should be merged after #23465That PR introduces a new block called
Social Icon
.Testing:
How has this been tested?
Social Icons
blockExpect: Social Icons is added with 4 icons, where WordPress icon is already fulfilled with the link and it's active (has product color)
Facebook
iconlink
icon in mobile toolbar above the keyboardURL
eg. facebook.com andLabel
Expect: Facebook social icon is active (has product color)
Social Icons
Expect: Only active social icons left
Screenshots
Types of changes
New feature:
Social Icons
blockChecklist: