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

PlainText: Add `multiline` and `isStylable` props #18238

Open
wants to merge 7 commits into
base: master
from

Conversation

@Copons
Copy link
Contributor

Copons commented Nov 1, 2019

Description

  • Add two new props to the PlainText component.
    • multiline: keeps the previous behaviour of allowing line breaks.
      If not set, it prevents line breaks in two ways: it replaces \n\r characters with spaces, and on press ENTER it insert a new default block instead.
    • isStylable: prepares the textarea to be styled with block attributes (e.g. color and background color).
      It also set the default background color of the textarea as transparent, to match the editor background color set by the theme.
  • Update the SiteTitle block to be a "stylable, single line" PlainText.
  • Update the Code, HTML, and Shortcode blocks (only ones that currently use PlainText) to be multiline PlainText, which is their original behaviour.

Notes

  • isStylable sounds very awkward, I know!
    I also know that it currently is hard to see its point, but I didn't want to add too much stuff in the SiteTitle component yet, to avoid making this PR more confusing.
    Though, its purpose can be tested by adding the .has-background.has-accent-background-color classes (in Twenty Twenty) to the textarea to simulate the background color block attribute.

  • I've chosen to add a backward-incompatible multiline instead of something like singleLine to keep the naming consistent with RichText etc. Though, I don't have a strong opinion on this, and I wouldn't be opposed to "invert" it.

How has this been tested?

Tested on the Gutenberg Docker with the Full Site Experiment enabled.
Checked if the new props caused regressions in other uses of the PlainText component.

Types of changes

Breaking change

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.
placeholder={ __( 'Site Title' ) }
value={ title }
onChange={ setTitle }
allowedFormats={ [] }
isStylable
onKeyDown={ preventNewlines }

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 2, 2019

Contributor

I think instead of onKeyDown here, we might want to align the API with how RichText works, I think it's onSplit or something like that cc @ellatrix

This comment has been minimized.

Copy link
@Copons

Copons Nov 4, 2019

Author Contributor

The difference here is that onSplit is an actual RichText method created for handling a block split, while in this case I'm just passing the native onKeyDown handler through to the textarea element.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 6, 2019

Member

This is another reason why I think it would be better to do this through (Rich)Text. The API's already there, and they would be the same.

onChange,
className,
multiline,
isStylable,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 2, 2019

Contributor

Should this be a prop, or should this be by default? I mean if users of PlainText don't want the styled behavior, they can always use TextAreaAutosize or just set styles on the wrapper? or do you think this behavior is too common to warrant a prop?

This comment has been minimized.

Copy link
@Copons

Copons Nov 4, 2019

Author Contributor

This is a good question.
Mainly, I wanted to affect as little as possible the other components using PlainText, to avoid introducing visual regressions that might be hard to catch.

At the same time, those other components currently use PlainText exactly like a normal textarea (plus the auto-resize, of course)... 🤔

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 2, 2019

multiline: keeps the previous behaviour of allowing line breaks.
If not set, it prevents line breaks in two ways: it replaces \n\r characters with spaces, and on press ENTER it insert a new default block instead.

Why not just use an input element?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 2, 2019

Why not just use an input element?

you still want the autosize behavior. Not sure how easy/redundunt this would be with an input.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 2, 2019

In that case, I'd recommend using RichText without any formatting, which has native autoresizing. Maybe we should consider wrapping it in another component for such use cases. The cool thing would be that you still have access to all other APIs such as splitting, autocomplete text... all without the text formatting.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 2, 2019

Maybe we should consider wrapping it in another component for such use cases. The cool thing would be that you still have access to all other APIs such as splitting, autocomplete text... all without the text formatting.

That's a great argument, I feel we shouldn't use RichText though as semantically it's about "Rich Text", and using code, you can always pass content with formatted content and it will be rendered as formatted content. It definitely means there's a lot of refactoring needed to extract/share things between a base Text component and two specialized RichText and PlainText. This is though a bigger refactor than just expanding the capabilities of PlainText step by step.

@Copons

This comment has been minimized.

Copy link
Contributor Author

Copons commented Nov 4, 2019

Why not just use an input element?

you still want the autosize behavior. Not sure how easy/redundunt this would be with an input.

@ellatrix @youknowriad AFAIK there is no way to make an input element wrap the text inside.

I'd recommend using RichText without any formatting

Yes, I really agree with you, and I argued for this solution at length in this thread: #17207 (review)
(and also anecdotally it's what I did weeks ago for the SiteTitle block in WordPress.com)

Though, it is my understanding that the proposed direction though would be to keep things as semantically correct as possible, so I went ahead with a very light implementation of a resizable single-line PlainText, slightly inspired by PostTitle.

It definitely means there's a lot of refactoring needed to extract/share things between a base Text component and two specialized RichText and PlainText.

To be honest at this point I don't think there is a need for such thing.
Since RichText already solves all the problems of any type of text input, it might make more sense to rename rich-text into text, and replace PlainText with a direct import of TextareaAutosize... 😅

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 6, 2019

you can always pass content with formatted content and it will be rendered as formatted content.

Not necessarily. In a normal text version, we wouldn't allow any formatting.

It definitely means there's a lot of refactoring needed to extract/share things between a base Text component and two specialized RichText and PlainText. This is though a bigger refactor than just expanding the capabilities of PlainText step by step.

The way I see it, this wouldn't be a big refactor at all.

I'm not sure if I'll find the time right now, but I can try to spin up a PR today or tomorrow.

@Copons

This comment has been minimized.

Copy link
Contributor Author

Copons commented Nov 6, 2019

I'm not sure if I'll find the time right now, but I can try to spin up a PR today or tomorrow.

@ellatrix I'd be very happy to see that! Let me know if I can be of any help, or just add me as reviewer. 🙇

@ellatrix ellatrix referenced this pull request Nov 7, 2019
0 of 5 tasks complete
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 7, 2019

#18361 introduces a Text component, which is RichText under the hood, but doesn't allow any formatting. In other words, it behaves just like a textarea when rendering the value.

<Text value="some <b>tags</b>" />

will be editable as

some <b>tags</b>

(normally those tags would be interpreted as formatting)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.