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

fix Toast stacking #419

merged 4 commits into from
Jan 6, 2017

Conversation

giladgray
Copy link
Contributor

Fixes #367

Checklist

  • Include tests
  • Update documentation

Changes proposed in this pull request:

  • clone Toast props before mutating. this avoids leaking the mutation and allows more than one of each Toast to appear in the example.
  • ensure Toasts are always position: relative so they also stack when inline.
  • warn when passing inline prop to Toaster.create()

Reviewers should focus on:

  • are you into this new warning or is it overkill? i updated the props docs with the same content.

Gilad Gray added 3 commits January 5, 2017 14:01
this avoids leaking the mutation and allows more than one of each Toast to appear in the example.
so they also stack when `inline`.
fixes #367
@giladgray giladgray requested a review from adidahiya January 5, 2017 22:19
@blueprint-bot
Copy link

fix lint

Preview: docs
Coverage: core | datetime

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Looks reasonable. 👍

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.

code looks ok; did not test in browsers

@@ -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!

@giladgray giladgray merged commit 248c1f1 into master Jan 6, 2017
@giladgray giladgray deleted the gg/toaster-inline branch January 6, 2017 18:33
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