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

Toaster create method should not have the inline property hard coded #367

Closed
tonico opened this issue Dec 14, 2016 · 8 comments
Closed

Toaster create method should not have the inline property hard coded #367

tonico opened this issue Dec 14, 2016 · 8 comments

Comments

@tonico
Copy link

tonico commented Dec 14, 2016

Bug report

  • Package version(s): 1.3.1
  • Browser and OS versions: all

Steps to reproduce

Create a custom Toaster with inline set to false, e.g., using code from the docs:

import { Position, Toaster } from "@blueprintjs/core";
 
export const OurToaster = Toaster.create({
    className: "my-toaster",
    position: Position.BOTTOM_RIGHT,
    inline: false
});

Actual behavior

Observe the pt-overlay-inline class in the generated DOM element:

<span data-reactroot="" class="pt-overlay pt-overlay-open pt-overlay-inline pt-toast-container pt-toast-container-top">...</span>

Expected behavior

The create method that returns a new Toaster instance should not have the inline property hard coded:
https://github.com/palantir/blueprint/blob/release-1.3.1/packages/core/src/components/toast/toaster.tsx#L94

@adidahiya
Copy link
Contributor

What are you trying to accomplish with inline: false?

@tonico
Copy link
Author

tonico commented Dec 14, 2016

From the docs: "If true, then positioning will be relative to the parent element". I think this is the reason why toasts are centered wrong (in Firefox). Also multiple toasts are not stacked one below the other (as seen in the docs) but on top of each other.

@giladgray
Copy link
Contributor

inline: false doesn't make sense when using Toaster.create because it circumvents the Portal logic and attaches the toaster to the element in the second argument (which defaults to body). This is the "global" API, where toasts will always appear above your application and there's nothing you can do about it.

However, <Toaster inline> (using the JSX API) makes perfect sense, if you'd like your toasts to live inside some element in your app.

@tonico
Copy link
Author

tonico commented Dec 14, 2016

Hm, OK, I understand. But the positioning issue remains, i.e., following the documentation

"You should use Toaster.create, rather than using the Toaster component API directly in React"

I would expect this: bildschirmfoto am 2016-12-14 um 19 19 06

But I'm getting this (in Firefox 50.0.2): bildschirmfoto am 2016-12-14 um 19 18 22

IMHO the create logic should be changed such that the result is like the expected behavior shown above.

@giladgray
Copy link
Contributor

giladgray commented Dec 14, 2016

Toasts on Firefox 50 on the docs site work as expected, so I wonder if you're adding any additional styles? Are you setting position: Position.BOTTOM_RIGHT like the example code above (cuz that would mess this up big time)? Could you share the usage code?

@tonico
Copy link
Author

tonico commented Dec 14, 2016

Yes, if this is the souce code of the docs, https://github.com/palantir/blueprint/blob/master/packages/core/examples/toastExample.tsx, then the docs do not use the Toaster.create() API. Also, inline is false by default. I'm creating a Toaster with const ErrorToaster = Toaster.create() without any position setting (the code above is borrowed from the docs). Later in the code I use

ErrorToaster.show({
    message: error.msg,
    timeout: 300000,
})

in a loop. As far as I can see in the Firefox DOM inspector, I do not override any CSS on the toast element or its container. My CSS is minimal anyway so I can post it here:

html,
body,
#root {
  margin: 0px;
  padding: 0px;
  height: 100%;
}

I also use styled-components, but not with toast elements.

@unp
Copy link

unp commented Dec 18, 2016

I'm having the same issue. I believe @tonico is right that the inline prop should not be hardcoded.

Currently, multiple Toasts are not stacking. I have the inline prop explicitly set to false in my Toaster.create method.

Toaster.create({ inline: false, container: document.body });

It creates the following reactroot:

<span data-reactroot="" class="pt-overlay pt-overlay-open pt-overlay-inline pt-toast-container pt-toast-container-top"><span>

If I remove the pt-overlay-inline class using my browser's inspector, the Toasts stack as expected.

@giladgray giladgray added this to the 1.5.0 milestone Jan 3, 2017
@giladgray giladgray self-assigned this Jan 5, 2017
@giladgray
Copy link
Contributor

The inline prop should be hardcoded enabled for Toaster.create() because that method always renders the Toaster into a new element appended to the container. Toaster.create({ inline: false }) would add two elements to the container: an empty div (the original container) and a .pt-portal into which the toaster actually renders.

I'm sorry if this doesn't make perfect sense--it's an implementation detail of Portal and I'm struggling to explain it clearly. Basically, Toaster.create() does its own portaling by manually appending an element, so we don't need another Portal doing its own append and wasting that original element.

The correct fix involves ensuring that .pt-toast is always position: relative so they'll stack properly regardless of context.

giladgray pushed a commit that referenced this issue Jan 5, 2017
so they also stack when `inline`.
fixes #367
@giladgray giladgray mentioned this issue Jan 5, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants