-
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
Implement list block in React Native #14636
Conversation
return { | ||
clientId: context.clientId, | ||
isSelected: context.isSelected, | ||
onFocus: context.onFocus, |
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.
In the web implementation context.setFocusedElement
is used instead to mark which RichText
component is active:
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 saw that but it looks we don't use the same logic on the native part on our block manager.
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.
how is going your solution work with multiple RichText
controls included in a single block?
Good question, but we are still not there. I agree that in the future we
will need to change our implementation to do the same has web. But at the
moment this is what we have available.
…On Wed, 27 Mar 2019 at 10:48, Grzegorz (Greg) Ziółkowski < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/block-editor/src/components/rich-text/index.native.js
<#14636 (comment)>:
> + clientId: context.clientId,
+ };
+ }
+ // When explicitly set as selected, use the value stored in the context instead.
+ if ( ownProps.isSelected === true ) {
+ return {
+ isSelected: context.isSelected,
+ clientId: context.clientId,
+ };
+ }
+
+ // Ensures that only one RichText component can be focused.
+ return {
+ clientId: context.clientId,
+ isSelected: context.isSelected,
+ onFocus: context.onFocus,
how is going your solution work with multiple RichText controls included
in a single block?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#14636 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnxUSRovo2txjSZxS_9iH--TSGzS2khks5va0x4gaJpZM4cLd7C>
.
|
Pushed 08318ca which includes a fix for the toggling of list type in nested lists. Known issues:
|
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.
Tested with Stefanos latest changes on iOS, this LGTM 👍
@hypest Could you perhaps also update the web version? |
* @return {boolean} True if the root list or nothing is selected, false if an | ||
* inner list is selected. | ||
*/ | ||
function isListRootSelected( value ) { |
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.
Why not replace getStartListFormat
with both isListRootSelected
and isActiveListType
? This seems weird to me.
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.
👋 @ellatrix , not sure I follow. Do you mean to completely remove getStartListFormat and implement its pieces inside isListRootSelected
and isActiveListType
?
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.
Yes, export isListRootSelected
and isActiveListType
from rich-text
as unstable.
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.
Done with 4451cb3.
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.
Would be great to update web version and also change the function names (it's ok to have to separate one for the two different purposes.
@@ -6,7 +6,7 @@ import { Toolbar } from '@wordpress/components'; | |||
import { __ } from '@wordpress/i18n'; | |||
import { | |||
changeListType, | |||
getStartListFormat, | |||
getLineListFormat, |
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 meant to rename it as getLineNestingLevel, back when it was only about the nesting. I think it's better to export two functions instead of overloading one for multiple purposes. 66e971d#r272474533
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.
Aha, I see. I will work on that.
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.
@hypest Ok. I thought it's useful to know that the functions you introduce work in both scenarios. |
Yes, that's perfect information to know @ellatrix , thanks for assessing this! |
Description
This PR implements the list block by only changing components and not touching the code of the block ( no need to create a native version).
This achieved by improving the implementation of the block-edit context in RNMobile in order to send the isSelected and onFocus props to the RichText components.
For the moment it doesn't implement the indent and outdent buttons, but those should be relatively after a better understanding of the format and selection code behind it.At the moment the block allows the breaking of the list by tapping two enters in a row on an empty list element. This can be solved by changing Aztec to send the event up instead of breaking the list by itself.How has this been tested?
This can be tested using this PR on gb-mobile: wordpress-mobile/gutenberg-mobile#704