-
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
Editable Component #18361
Editable Component #18361
Conversation
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.
Any thoughts about PlainText? Should we deprecate it, should we create a shortcut?
I think with (@ellatrix I'm so sorry for not having reviewed this yet, but I've been swamped with new urgent tasks!) |
@@ -65,6 +65,7 @@ class RichTextWrapper extends Component { | |||
this.onPaste = this.onPaste.bind( this ); | |||
this.onDelete = this.onDelete.bind( this ); | |||
this.inputRule = this.inputRule.bind( this ); | |||
this.getAllowedFormats.EMPTY_ARRAY = []; |
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 just a module constant
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 empty array should be used specifically for the allowed formats state. I don't see a reason to make it a module constant. It isn't and shouldn't be reused for other state.
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 having a more declarative name be of any help? E.g. this.getAllowedFormats.NO_FORMATS = []
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 can also use a module variable and name it differently. I just have this habit of putting stuff close to what is using it.
const { | ||
allowedFormats, | ||
formattingControls, | ||
__unstableDisableFormats, |
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.
Instead of having these disbleFormats props which doesn't make sense for a RichText component, ideally RichText should be a special version of Text. how feasable is 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.
- Why does it not make sense for
RichText
? I think it's fine. - It's harder right now to make
RichText
a wrapper aroundText
, but yes, in the future this could be done. To keep things simple with a minimum amount of code changes, I chose to useRichText
internally inText
, but that can be refactored later.
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 guess we could just make it a stable prop and omit __unstable
. Maybe I'm being too prudent in introducing new props.
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 agree with @ellatrix.
This is a great starting point: it makes things very straightforward, and doesn't prevent an eventual Text
/RichText
"switcheroo".
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 my mind ideally disableFormats could be a valid prod for a low-level Text component that is used by higher-level PlainText and RichText components. So I think it doesn't make sense for RichText because RichText by definition is for Rich Text which means "formats".
I get that the refactor can be achieved later and is probably not that easy so unstable is better.
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.
@youknowriad Oh, I'm sorry, I confused disableFormats
and disableLinebreaks
. I thought you were talking about the line breaks. Sorry about that. You're right, the prop doesn't make sense on RichText
at all, it's just a stepping stone for now.
onChange( insert( value, '\n' ) ); | ||
if ( ! disableLineBreaks ) { | ||
onChange( insert( value, '\n' ) ); | ||
} |
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.
While reading the changes here, I was wondering why these onEnter handlers are block-editor specific and not something for the regular RichText component?
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.
Inserting line breaks on enter could be a default feature of RichText
(in the rich text package), sure. But it should also be overridable. Right now there's just no default handling. Maybe in a future PR.
*/ | ||
import RichText from '../rich-text'; | ||
|
||
function Text( props ) { |
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.
Should we name this PlainText instead.
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.
See #18361 (comment).
@youknowriad @Copons |
If the APIs are compatible why not just replace the existing PlainText? |
@youknowriad because, until now, |
packages/block-editor/README.md
Outdated
@@ -422,6 +422,10 @@ _Type_ | |||
|
|||
- `Object` | |||
|
|||
<a name="Text" href="#Text">#</a> **Text** | |||
|
|||
Renders an editable text input in which text formatting is not allowed. |
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 think this might be misinterpreted as an <input type="text">
.
What about being more candid and go with something like Renders a contenteditable element in which text formatting is not allowed
?
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, we need better descriptions. The mobile team previously reminded me not to use contenteditable
as that's not the case for the native apps. @hypest @SergioEstevao @Tug and maybe others (not sure if @WordPress/native-mobile works), what would be a good description? :)
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 should also update the RichText
description. :)
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.
Also Text
might be confusing because in react-native it's a display-only component. For this, react-native uses TextInput:
A foundational component for inputting text into the app via a keyboard. Props provide configurability for several features, such as auto-correction, auto-capitalization, placeholder text, and different keyboard types, such as a numeric keypad
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.
Sorry I'm a bit late to the conversation, I'd vote for TextInput
since it would have very similar behavior as the react-native component @koke talked about.
If PlainText is just an autosizeable textarea, there's no need for a component for it, you can just use TextareaAutosize. That the reason I think naming this |
@@ -0,0 +1,114 @@ | |||
# `Text` | |||
|
|||
Renders an editable text input in which text formatting is not allowed. |
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.
@youknowriad Ok, sounds good to me. I was just being careful. :) It's worth noting though that other (not listed) |
What about keeping |
@youknowriad @Copons Looking at
I would feel more comfortable starting a new component. |
@@ -73,6 +74,7 @@ class RichTextWrapper extends Component { | |||
onSplit, | |||
multiline, | |||
markAutomaticChange, | |||
disableLineBreaks, |
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 prop addressed #13570.
@ellatrix, @youknowriad: what about a deprecation cycle like so:
|
@mcsf That sounds interesting. I think I like it. it does create a precedent in terms of backward compatibility handling. At the moment, deprecated calls are never removed. I think they should be at some point, but it's hard to have these discussions properly, how many versions should we wait before removing the deprecated usage... This is though a very "smallish" breaking change (just a styling one), so I don't think it would be a huge deal. |
@hypest @SergioEstevao Why are there |
So I had a look on this and we don't need to use props we could get them from the style object.
|
@SergioEstevao What does font family need a separate prop? |
The site title block looks good to me. |
Size Change: -889 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
Let's do this and iterate. |
I personally still have concerns on the naming, and since this is a new API, maybe we should take time to discuss it properly. |
@youknowriad Ok, sorry. It's just been sitting around for a long time and I didn't feel there were big objections. Shall I revert? |
@ellatrix funny enough I was looking at the PR today before you merge it :) The I'm still leaning towards |
Well, if |
It seems we don't agree on the |
I don't find the
|
Why not call this component "EditableText"? "Editable" isn't that clear; editable what? |
Ideally, we find a solution here before the next plugin release (monday). |
I’ll rename the component today and alias it to plaintext version 2?
Op vr 20 mrt. 2020 om 11:12 schreef Riad Benguella <[email protected]
…
Ideally, we find a solution here before the next plugin release (monday).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18361 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABD6B2ZOU2CRFOQTKEJC5SLRIM6RDANCNFSM4JKDJLHQ>
.
|
@ellatrix That works for me but it can wait until Monday, I know you're AFK today. |
Description
Alternative to #18238.
Fixes #13570.
This PR introduces a new
Editable
component, which purpose is to edit text without any formatting. It won't allow any formatting, nor will it have formatting controls. If any HTML tags are present, they are just rendered as text, not as text formatting.The behaviour is much the same as a plain
textarea
, but there are a few advantages:RichText
APIs:onSplit
,onReplace
,onRemove
,onMerge
.RichText
's, which means it could be used for collaborative editing.RichText
.Under the hood, it's just
RichText
where formats are disabled, since a lot of the code can be shared. In the future, we could refactor this a bit and split out all the formatting logic, but for now, this works just fine. I could seeEditable
become the base component forRichText
.Sharing the codebase will eventually make the experience more streamlined. API improvements for text will benefit rich text, and improvements for rich text will benefit text.
I updated the site title block to use this component.
How has this been tested?
Try out the site title block.
Screenshots
Types of changes
Checklist: