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

FormTokenField: Make it possible to add children #19676

Closed

Conversation

scruffian
Copy link
Contributor

Description

This makes it possible to add children to FormTokenField. There are cases where we want to use this component within a form and we'd like to add a button, but it's hard to do so. This PR will make it simpler

How has this been tested?

Try adding a child component to FormTokenField

Screenshots

This adds a "test" paragraph
Screenshot 2020-01-15 at 16 13 29

Types of changes

New feature (non-breaking change which adds functionality)

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. .

@aduth
Copy link
Member

aduth commented Jan 22, 2020

There are cases where we want to use this component within a form and we'd like to add a button, but it's hard to do so.

Can you elaborate on these use-cases?

I think the screenshot as presented appears problematic, though I suppose that could be a result of it intending to be a contrived example. It seems problematic because it disassociates the help text from the field input. I think it's worth considering in how: (a) If this is the default behavior, then it sets up developers for failure (to easily introduce negative UX implications) and (b) Whether there might be a better way to abstract this if the intention is something more like "adding buttons alongside [the screenshot's "Embed" button]" (e.g. something like a buttons or actions prop, etc).

@karmatosed karmatosed added the [Package] Components /packages/components label Jan 22, 2020
@scruffian
Copy link
Contributor Author

@aduth This is taken from a real example in Jetpack - the OpenTable block. In this case we are adding the "Embed" button after the field and then positioning it on the right (as in the screenshot above) with a lot of fragile CSS. Instead if we could insert the button into the markup it would be easier to align it.

Rather than allow any children we could instead just have a prop which adds a button. What do you think?

@aduth
Copy link
Member

aduth commented Jan 22, 2020

What does the "Embed" button do? Is it something like a <button type="submit"> ?

If it were, then just like submit buttons in a form, I don't think it would make sense as embedded in the input itself. I could imagine a layout like the one in the screenshot, but I would view it more like a flex row of:

+---------------+----------+
| [Input]       | [Submit] |
| Help Text     |          |
+---------------+----------+

@scruffian
Copy link
Contributor Author

This is the way we approached it in the OpenTable block:
https://github.com/Automattic/jetpack/blob/c433ab7ff6bf42c36a4a3c725009c93b0a183587/extensions/blocks/opentable/editor.scss

Now I look at it, it's not that much additional CSS. I guess we don't need this after all :)

@scruffian scruffian closed this Jan 22, 2020
@scruffian scruffian deleted the update/form-token-field branch January 22, 2020 16:12
@aduth
Copy link
Member

aduth commented Jan 22, 2020

I looked a little closer at how it's used in the OpenTable block (Automattic/jetpack#14220). If I understand correctly, the field can also accept embed code, and the button would interpret the input value as embed code?

If that's the case, I think at that point it can't really be considered a token field. Or at least it's more of an overloaded field which can be used either for tokens, or for this custom code. Personally I might worry about the usability of this approach vs. separate inputs, but I think it would be fine to consider it as something different from a token field. It would be more of a custom implementation and not really a valid use-case for the scope of what this component is intending to support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants