-
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
[Mobile] List V2 Support #42702
[Mobile] List V2 Support #42702
Conversation
…r inner blocks and selecting a native TagName
…other data like fontSize/lineHeight
Size Change: +1.52 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
* Mobile - List V2: - List Item - Add support for list type for both ordered and standard icon. - List - Avoid passing the start prop on Mobile * Mobile - ListStyleType - Fix default color * Mobile - ListItem - Add margin for parent list * Mobile - ListItem - Check if there's a default font size from the theme * Mobile - ListItem - Add deleteEnter for RichText * Mobile - ListItem - Remove unneeded theme fontSize parsing * Mobile - ListItem - Fix list margins * Mobile - ListItem - Fix margins * Mobile - Global styles - Parse font sizes * Mobile - List Style - Use theme.json font size as a default if its a block based theme
@@ -15,7 +15,7 @@ import settingsV2 from './v2'; | |||
|
|||
const { name } = metadata; | |||
|
|||
export { metadata, name }; | |||
export { metadata, name, settingsV2 }; |
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 will be needed just for the mobile release with the feature flag, once the feature flag is removed, this won't be needed.
@@ -165,7 +172,7 @@ function Edit( { attributes, setAttributes, clientId } ) { | |||
<> | |||
<TagName | |||
reversed={ reversed } | |||
start={ start } | |||
start={ Platform.isWeb ? start : undefined } |
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.
Passing start
to the View
component on mobile breaks the styling of the list
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.
Does that only happen for ul
, or for both?
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.
Might be good to have a separate file for this tag name for native and web.
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.
Does that only happen for ul, or for both?
It happens for both.
Might be good to have a separate file for this tag name for native and web.
That's a good idea! I'll update it 👍
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.
@@ -304,7 +304,9 @@ export class BlockList extends Component { | |||
} | |||
title={ title } | |||
ListHeaderComponent={ header } | |||
ListEmptyComponent={ ! isReadOnly && this.renderEmptyList } | |||
ListEmptyComponent={ | |||
isRootList && ! isReadOnly && this.renderEmptyList |
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.
We shouldn't be rendering this for inner blocks since it renders a blank space, this was breaking the new List V2 format. So now it will only render if it's the root list.
@@ -30,7 +30,7 @@ import { | |||
} from './hooks'; | |||
import { convertToListItems } from './utils'; | |||
|
|||
function IndentUI( { clientId } ) { | |||
export function IndentUI( { clientId } ) { |
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 export is needed to share it with the mobile variant of the ListItem
block
@geriux I noticed that when changing the font size on Android, there seems to be an issue with line height on large font sizes. The text appears to be cut off at the top. When you tap one of the cut off pieces of text and start typing, the line height returns to normal. This doesn't happen on iOS. Going to investigate further, but just wanted to see if you had any thoughts too. Video ExampleIndented.list.line.height.2.mov |
const blockProps = useBlockProps(); | ||
function Edit( { attributes, setAttributes, clientId, style } ) { | ||
const blockProps = useBlockProps( { | ||
...( Platform.isNative && { style } ), |
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.
What is this used for?
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 used to inherit the styles, mostly used for the color text/background customization and global styles.
const innerBlocksProps = useInnerBlocksProps( blockProps, { | ||
allowedBlocks: [ 'core/list-item' ], | ||
template: TEMPLATE, | ||
templateInsertUpdatesSelection: true, | ||
...( Platform.isNative && { marginVertical: 8, marginHorizontal: 8 } ), |
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.
What is the default here? Why is this set with inner block props and not CSS?
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 default values are 16
for both margins, this is used in the BlockList to calculate some block spacings. We do something similar for the Gallery block, maybe I could move the 8
value to a variable so we don't have a random number there. What do you think?
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.
Looks fine for web.
Hey @derekblank thanks for sharing that issue, I'll look into it, I think I know what's happening there, I'll work on a fix for that 👍 |
…ight updates weren't showing correctly due to issues with the views layout
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.
Confirming the Android line height issue is resolved with the most recent changes. Adding another approval -- LGTM! 🚀
Main PRs:
Gutenberg Mobile
-> List V2 Support wordpress-mobile/gutenberg-mobile#5054WordPress iOS
-> Gutenberg - Add list block v2 flag wordpress-mobile/WordPress-iOS#19116WordPress Android
-> [Gutenberg] Add List Block V2 flag wordpress-mobile/WordPress-Android#16962Fixes #42624
Related PRs:
__experimentalEnableListBlockV2
flag for mobile #42697What?
This PR adds support for the new List V2 block on mobile, including enabling this new version using the
__experimentalEnableListBlockV2
feature flag that will be fetched through the block editor settings endpoint.Why?
To keep compatibility with the new changes in the web editor and prevent issues when rendering a block format that is not supported.
How?
Adds compatibility sharing the same code with Web for the edit.js file, with some required changes like platform-specific TagName and styles.
It adds the new
ListItem
component, this one has its own native variant since it required quite a few changes making it harder to share with Web, although it does uses shared code like theIndentUI
component.It also introduces a new component
ListStyleType
this was reviewed in a separate PR to make it easier to review, you can see just those changes in this PR. This new component will take care of the list styling, whether if its an ordered list or a standard one with their different indentation levels.Introduces the
__experimentalEnableListBlockV2
feature flag logic as we did for the Quote V2 block, this will be in the code base for a brief time so we could make a mobile release with the new format disabled and later on activate it once the Web has it available through the Gutenberg plugin. In a following mobile release, this feature flag logic will be removed and List V2 block will be enabled by default.Lastly, it introduces some Global styles changes we did in this PR, to parse font size values that were in a different format, like
rem
values.Unit tests have been added for the new List V2 format and kept two simple ones for the older version.
Testing Instructions
Precondition: A site with the Gutenberg 13.8 plugin.
NOTE: Once the flag is enabled sometimes the new List block won't be shown the first time the editor is opened, so if you still see the old version without inner blocks, close the editor and open it again.
Test case 1 - Test different levels of indentation
Test case 2 - Test different levels of indentation and ordered lists
reverse
optionTest case 3 - Set a different font size
Test case 4 - Set some background and text colors
Screenshots or screencast
Test case 1 - Test different levels of indentation
Test case 2 - Test different levels of indentation and ordered lists
Test case 3 - Set a different font size
Test case 4 - Set some background and text colors