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

Toast docs + children support #2048

Merged
merged 5 commits into from
Jan 29, 2018
Merged

Toast docs + children support #2048

merged 5 commits into from
Jan 29, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Jan 29, 2018

Fixes #1224

Changes proposed in this pull request:

  • Toaster renders children after state toasts
  • 🐛 Overlay now passes through non-JSX children
    • string | number children will not be animated. to be fair, this is an improvement over the previous behavior where it would simply die if given non-JSX children.
  • major refactors to Toasts documentation to clarify expected usage (see Toast api seems to violate core React principles #1224)

@giladgray giladgray changed the title Toast docs Toast docs + children support Jan 29, 2018
@giladgray
Copy link
Contributor Author

Question:

Should Toaster support a toasts prop just like its state?
And I'll use lifecycle methods to keep them in sync.
Or is children enough?

(my gut is children is enough.)

@blueprint-bot
Copy link

Toast docs: clarify usage

Preview: documentation | landing | table

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

toasts don't work in the docs preview

aligned along the top or bottom edge of its container (new toasts will slide in from that edge) and
horizontally aligned along the left edge, center, or right edge of its container.
Render the `<Toaster>` component like any other element. Then, either access the instance methods by supplying
a `ref` handler (see example below), or supply rendered `<Toast>` elements as `children` of the `<Toaster>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we're suggesting a ref handler... if someone really wanted to do this in a React-idiomatic way, wouldn't they provide toasts as children? What are the benefits of this over the Toaster.create() API? I feel all these options make the usage docs more confusing.

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'll move the children suggestion first and note that you can ref but please don't (see next paragraph).

@@ -58,51 +51,71 @@ OurToaster.update(key, { message: "Still toasted!" });
enable `autoFocus` for a `Toaster` via a prop, if desired.
</div>

@### Static method
@interface IToaster
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go inside the "Static usage" section, after the first bit of prose there. Prefer reading prose first, detailed props tables later

@blueprint-bot
Copy link

Overlay supports falsy children

Preview: documentation | landing | table

@blueprint-bot
Copy link

docs

Preview: documentation | landing | table

`Toaster` can be used in one of two ways:

1. `Toaster.create(props)` static method returns a new `IToaster` instance. Use the instance method `toaster.show()` to manipulate this instance. __(recommended)__
1. Render a `<Toaster>` element and either use the `ref` prop to access its instance methods, or pass rendered `<Toast>`s as `children`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like 3 options.

  1. Toaster.create(...)
  2. <Toaster ref={...} />
  3. <Toaster><Toast ... /></Toaster>

they can all be mixed & matched anyway, so might as well list all 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 and 3 can be mixed, no way to provide children to 1.

but yes there are three, so i'll update

Copy link
Contributor

Choose a reason for hiding this comment

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

well you can combine option 1 with 2 & 3, you'd just get multiple Toaster instances

@blueprint-bot
Copy link

three ways to use a toaster

Preview: documentation | landing | table

@giladgray giladgray merged commit 5b6961f into develop Jan 29, 2018
@giladgray giladgray deleted the gg/toast-docs branch January 29, 2018 23:04
@rrcobb
Copy link

rrcobb commented Jan 29, 2018

Love it, thanks for following through!

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