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.

Sign up for GitHub

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

Add list props to rich text in native. #18748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
761e503
Add list props to rich text in native.
SergioEstevao Nov 25, 2019
0fb5bfd
Merge branch 'refs/heads/master' into rnmobile/list-settings
SergioEstevao Nov 26, 2019
c26f4e7
Merge branch 'refs/heads/master' into rnmobile/list-settings
SergioEstevao Nov 27, 2019
3d176da
Merge branch 'refs/heads/master' into rnmobile/list-settings
SergioEstevao Nov 27, 2019
23e4728
Make sure we remove root tags with extra attributes too
SergioEstevao Nov 27, 2019
2fc6d6d
Adapt the keyboard of the control depending of the type of the control.
SergioEstevao Nov 27, 2019
bff7d16
Make sure component refresh when props start/reversed change.
SergioEstevao Nov 27, 2019
0c4877e
Merge branch 'refs/heads/master' into rnmobile/list-settings
SergioEstevao Nov 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,8 @@ class RichTextWrapper extends Component {
style={ style }
preserveWhiteSpace={ preserveWhiteSpace }
disabled={ disabled }
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.

>
{ ( { isSelected, value, onChange, Editable } ) =>
<>
Expand Down
3 changes: 0 additions & 3 deletions packages/block-library/src/list/ordered-list-settings.native.js
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions packages/components/src/mobile/bottom-sheet/cell.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ class BottomSheetCell extends Component {
this.setState( { isScreenReaderEnabled } );
}

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

@etoledom etoledom Nov 27, 2019 •
edited
Loading

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:

gutenberg/packages/components/src/mobile/bottom-sheet/cell.native.js

Line 174 in bff7d16

{ ...valueProps }

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 👍


render() {
const {
accessible,
Expand All @@ -80,6 +92,8 @@ class BottomSheetCell extends Component {
style = {},
getStylesFromColorScheme,
customActionButton,
type,
step,
...valueProps
} = this.props;

Expand Down Expand Up @@ -156,6 +170,7 @@ class BottomSheetCell extends Component {
pointerEvents={ this.state.isEditingValue ? 'auto' : 'none' }
onFocus={ startEditing }
onBlur={ finishEditing }
keyboardType={ this.typeToKeyboardType( type, step ) }
{ ...valueProps }
/>
) : (
Expand Down
17 changes: 14 additions & 3 deletions packages/rich-text/src/component/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ export class RichText extends Component {
}

removeRootTag( tag, html ) {
const openingTagRegexp = RegExp( '^<' + tag + '>', 'gim' );
const openingTagRegexp = RegExp( '^<' + tag + '[^>]*>', 'gim' );
const closingTagRegexp = RegExp( '</' + tag + '>$', 'gim' );
return html.replace( openingTagRegexp, '' ).replace( closingTagRegexp, '' );
}
Expand Down Expand Up @@ -491,7 +491,9 @@ export class RichText extends Component {
}

shouldComponentUpdate( nextProps ) {
if ( nextProps.tagName !== this.props.tagName ) {
if ( nextProps.tagName !== this.props.tagName ||
nextProps.reversed !== this.props.reversed ||
nextProps.start !== this.props.start ) {
this.lastEventCount = undefined;
this.value = undefined;
return true;
Expand Down Expand Up @@ -600,7 +602,16 @@ export class RichText extends Component {
}

if ( tagName ) {
value = `<${ tagName }>${ value }</${ tagName }>`;
let extraAttributes = ``;
if ( tagName === `ol` ) {
if ( this.props.reversed ) {
extraAttributes += ` reversed`;
}
if ( this.props.start ) {
extraAttributes += ` start=${ this.props.start }`;
}
}
value = `<${ tagName } ${ extraAttributes }>${ value }</${ tagName }>`;
}
return value;
}
Expand Down