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

fix Toast stacking #419

Merged
merged 4 commits into from
Jan 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/core/src/common/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ export const RANGESLIDER_NULL_VALUE = `${ns} <RangeSlider> value prop must be an
export const TABS_FIRST_CHILD = `${ns} First child of <Tabs> component should be a <TabList>`;
export const TABS_MISMATCH = `${ns} Number of <Tab> components should equal number of <TabPanel> components`;

export const TOASTER_INLINE_WARNING = `${ns} Toaster.create() ignores inline prop as it always creates a new element`;

export const WARNING_DIALOG_NO_HEADER_ICON = `${ns} Warning: Dialog iconName prop is ignored if title prop is omitted`;
export const WARNING_DIALOG_NO_HEADER_CLOSE_BUTTON =
`${ns} Warning: Dialog isCloseButtonShown prop is ignored if title prop is omitted`;
4 changes: 3 additions & 1 deletion packages/core/src/components/toast/_toast.scss
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ $toast-margin: $pt-grid-size * 1.5 !default;
display: flex;
align-items: flex-start;

position: relative;
// toasts rely on relative positioning for stacking; override inline overlay styles (#367)
// stylelint-disable-next-line declaration-no-important
position: relative !important;
margin: $toast-margin 0 0;
border-radius: $pt-border-radius;
box-shadow: $pt-elevation-shadow-3;
Expand Down
22 changes: 17 additions & 5 deletions packages/core/src/components/toast/toaster.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ import * as ReactDOM from "react-dom";

import { AbstractComponent } from "../../common/abstractComponent";
import * as Classes from "../../common/classes";
import { TOASTER_INLINE_WARNING } from "../../common/errors";
import { ESCAPE } from "../../common/keys";
import { Position } from "../../common/position";
import { IProps } from "../../common/props";
import { safeInvoke } from "../../common/utils";
import { safeInvoke, shallowClone } from "../../common/utils";
import { Overlay } from "../overlay/overlay";
import { IToastProps, Toast } from "./toast";

Expand Down Expand Up @@ -59,6 +60,9 @@ export interface IToasterProps extends IProps {
/**
* Whether the toaster should be rendered inline or into a new element on `document.body`.
* If `true`, then positioning will be relative to the parent element.
*
* This prop is ignored by `Toaster.create()` as that method always appends a new element
* to the container.
* @default false
*/
inline?: boolean;
Expand Down Expand Up @@ -89,6 +93,9 @@ export class Toaster extends AbstractComponent<IToasterProps, IToasterState> imp
* The `Toaster` will be rendered into a new element appended to the given container.
*/
public static create(props?: IToasterProps, container = document.body): IToaster {
if (props != null && props.inline != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would prefer explicit checks against undefined; we don't really need to support nulls

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'm just so used to doing it this way.

how about if we tackle that change across the codebase when we enable strictNullChecks?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but it won't get flagged for you automatically; you'll need to look for all such comparisons against null. in my ideal world, we also enable the no-null-keyword lint rule and any explicit usage of null would have to be whitelisted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure that sounds reasonable!

console.warn(TOASTER_INLINE_WARNING);
}
const containerElement = document.createElement("div");
container.appendChild(containerElement);
return ReactDOM.render(<Toaster {...props} inline /> , containerElement) as Toaster;
Expand All @@ -102,17 +109,15 @@ export class Toaster extends AbstractComponent<IToasterProps, IToasterState> imp
private toastId = 0;

public show(props: IToastProps) {
const options = props as IToastOptions;
options.key = `toast-${this.toastId++}`;
const options = this.createToastOptions(props);
this.setState((prevState) => ({
toasts: [options, ...prevState.toasts],
}));
return options.key;
}

public update(key: string, props: IToastProps) {
const options = props as IToastOptions;
options.key = key;
const options = this.createToastOptions(props, key);
this.setState((prevState) => ({
toasts: prevState.toasts.map((t) => t.key === key ? options : t),
}));
Expand Down Expand Up @@ -171,6 +176,13 @@ export class Toaster extends AbstractComponent<IToasterProps, IToasterState> imp
return <Toast {...toast} onDismiss={this.getDismissHandler(toast)} />;
}

private createToastOptions(props: IToastProps, key = `toast-${this.toastId++}`) {
// clone the object before adding the key prop to avoid leaking the mutation
const options = shallowClone<IToastOptions>(props);
options.key = key;
return options;
}

private getPositionClasses() {
const positions = Position[this.props.position].split("_");
// NOTE that there is no -center class because that's the default style
Expand Down
14 changes: 14 additions & 0 deletions packages/core/test/toast/toasterTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,18 @@ describe("Toaster", () => {
toaster.clear();
assert.isTrue(onDismiss.calledOnce, "onDismiss not called");
});

it("reusing props object does not produce React errors", () => {
const errorSpy = sinon.spy(console, "error");
// if Toaster doesn't clone the props object before injecting key then there will be a
// React error that both toasts have the same key, because both instances refer to the
// same object.
const toast = { message: "repeat" };
toaster.show(toast);
toaster.show(toast);
assert.isFalse(
errorSpy.calledWithMatch("two children with the same key"),
"mutation side effect!",
);
});
});