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

[TagInput] values accepts any valid JSX #1419

Merged
merged 2 commits into from
Aug 9, 2017
Merged

[TagInput] values accepts any valid JSX #1419

merged 2 commits into from
Aug 9, 2017

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Aug 8, 2017

Addresses #1412

Changes proposed in this pull request:

values array now accepts React.ReactNode[], which is actually the same type as children! not using children for this makes the array nature explicit. One cool thing is you can now pass an array to do inline styling: values: ["Barney", ["Bar", <em>thol</em>, "emew"], "Bertha"]

had to widen the types of a few props to support this (but actually added only one line of actual code), so I added some notes to props docs about how you can declare your own simpler types on your handlers.

Reviewers should focus on:

  • do we actually want this? the fact that i had to change no code and everything just works suggests, "why not!"
  • ReactNode is basically the most permissive type out there. Do we want to be a little more strict like string | JSX.Element?

Screenshot

image

with lots of notes about how to simplify your typescript code
@blueprint-bot
Copy link

TagInput values accepts any valid JSX with lots of notes about how to simplify your typescript code

Preview: documentation
Coverage: core | datetime

@giladgray giladgray mentioned this pull request Aug 8, 2017
8 tasks
Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

JSX.Element is probably too broad totally fine, and useful to support bold, italics and such 👍

@adidahiya
Copy link
Contributor

Do we use ReactNode anywhere else in the public API? What about JSX.Element? We should probably standardized on one for "renderable things". ReactNode seems fine.

@giladgray
Copy link
Contributor Author

@adidahiya we use ReactNode rarely in public API: Documentation navbarLeft/Right in docs package and Checkbox,etc labelElement which was added only recently #1378. this is definitely an experiment to see if ReactNode works in the public API, and it seems like it does.

that said, the difference btw ReactNode and string | JSX.Element is minimal. the upside of the latter is a more legible and grokable API, the downside is having to wrap formatting in an extra element. thoughts?

@blueprint-bot
Copy link

comments on values types

Preview: documentation
Coverage: core | datetime

@adidahiya
Copy link
Contributor

wrap formatting in an extra element

how so? isn't this the case with ReactNode as well?

@giladgray
Copy link
Contributor Author

@adidahiya it is not the case with ReactNode: you can supply an array of mixed strings and elements.

@adidahiya
Copy link
Contributor

ok, makes sense. ReactNode still sgtm then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants