Skip to content
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

Add list props to rich text in native. #18748

Merged
merged 8 commits into from
Dec 4, 2019
Merged

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Nov 26, 2019

Description

Implemented list-settings for the mobile project in RN.

@ellatrix can you check the way I'm passing reversed and start props, I found it curious the way we are passing those to richtext in the web version.

How has this been tested?

This can be tested using this PR in GB-Mobile: wordpress-mobile/gutenberg-mobile#1619

Screenshots

Simulator Screen Shot - iPhone 11 Pro Max - 2019-11-26 at 12 08 18

Types of changes

Compatibility with the mobile app RN version.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@SergioEstevao SergioEstevao added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 26, 2019
@SergioEstevao SergioEstevao added this to the Future milestone Nov 26, 2019
@@ -400,6 +400,8 @@ class RichTextWrapper extends Component {
__unstableUndo={ undo }
style={ style }
preserveWhiteSpace={ preserveWhiteSpace }
start={ start }
reversed={ reversed }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both these props are already added further down to Editable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make that work for native? Ideally all not rich text related props are directly passed to the editable element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't have editable on native, and all of the props need to go trough rich text.

One thing that I was thinking was to create an TagAttributes property that will go along with TagName prop and controlled by the blocks.

What do you think @ellatrix ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to create an Editable on native?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be possible, but it will take some considerable work to get there, and out of scope for this PR.

We basically will need to make our Native Aztec rich text component to be the Editable and respect all the API for it.

@koke and @hypest may have more thoughts about this, and when to give it to priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked with @ellatrix and we agree to keep this as it is, and tackle it on a separate PR.

Comment on lines +60 to +70
typeToKeyboardType( type, step ) {
let keyboardType = `default`;
if ( type === `number` ) {
if ( step && Math.abs( step ) < 1 ) {
keyboardType = `decimal-pad`;
} else {
keyboardType = `number-pad`;
}
}
return keyboardType;
}
Copy link
Contributor

@etoledom etoledom Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking first to have this in TextControl, but if it's something that could be used in other instances of Cell I think it's fine to be here.

I'm wondering, what is step used for?
The settings sends a step="1" and it seems to not be used by the Web TextControl or html textarea 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see from the web the step prop is passed directly to HTML input element.
On the web step is used like this: https://www.w3schools.com/tags/att_input_step.asp

And from what I see TextControl on native uses Cell directly, how do you see it I can use it on the TextControl level of native?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And from what I see TextControl on native uses Cell directly, how do you see it I can use it on the TextControl level of native?

Any extra prop we pass to Cell, will be added to the Cell's internal TextInput component via the valueProps here:

So if we pass keyboardType to Cell in TextControl it should work. Since Cell is more generic and TextControl is meant to be used as the text input component, maybe it would make sense. But I'm fine with keeping it in Cell how it is now too. Give a thought and you can decide :)

Thanks for the explanation, I didn't see the prop on the textarea docs but I see the arrows up and down in the Web settings UI, so that makes sense 👍

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on iOS and works perfect 🎉
Thanks @SergioEstevao for a great work!

Let's be sure to not merge until we decide what to do with the Android implementation 👍

# Conflicts:
#	packages/block-editor/src/components/rich-text/index.js
@SergioEstevao SergioEstevao merged commit c9242ac into master Dec 4, 2019
@SergioEstevao SergioEstevao deleted the rnmobile/list-settings branch December 4, 2019 14:33
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants