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.show(props, key?) #1962

Merged
merged 5 commits into from
Jan 25, 2018

Conversation

mcintyret
Copy link
Contributor

fixes #1894

}
return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since Array.find() doesn't seem to be available.

Copy link
Contributor

Choose a reason for hiding this comment

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

could do it with a filter().length > 0

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

looks good, but do we even need update if show can handle all the key logic?

}
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

could do it with a filter().length > 0

this.setState(prevState => ({
toasts: [options, ...prevState.toasts],
}));
return options.key;
}

public showOrUpdate(key: string, props: IToastProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if this method (and, by extension, update) is really necessary or it it can all be accomplished through show(props, key)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that this could replace show(). The semantics for update() are different - update() may not result in a toast being show, while show() and showOrUpdate() always do. That said, I'm not sure the semantics of update() are all that useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda dig the single show method that'd do an update if the key already exists, seems pretty natural no? Also with the code below, I feel like showOrUpdate should actually be called updateOrShow for clarity

*
* Callers may optionally supply a key, or a unique key will be generated.
*
* Returns the unique key of the new toast.
Copy link
Contributor

Choose a reason for hiding this comment

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

one paragraph should be sufficient. remove blank lines.

@giladgray
Copy link
Contributor

@mcintyret can we trouble you to change this feature so there's only the one show() method which accepts an optional key as its second argument?

@giladgray
Copy link
Contributor

@mcintyret any update here? the team can pick this up if you don't have the bandwidth, just let us know!

@mcintyret
Copy link
Contributor Author

@giladgray sorry was on vacay.

Just to be clear - there would be one show method with functionality equivalent to showOrUpdate here, and we get rid of showOrUpdate and update?

@adidahiya adidahiya changed the base branch from master to develop January 15, 2018 17:11
@giladgray
Copy link
Contributor

@mcintyret yes, one show method should be sufficient

@mcintyret
Copy link
Contributor Author

Hmm - tests passed for me locally, and the one that failed was resizeRowsByTallestCell - doesn't seem related?

@giladgray
Copy link
Contributor

@mcintyret yeah that test is not related to this change. rerunning failed jobs on circle, let's see if it's a flake.

@mcintyret
Copy link
Contributor Author

No luck :-( I'm seeing the same test fail in my other PR

@mcintyret
Copy link
Contributor Author

Mind poking this again?

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

code change looks fine but what's up with that test?

@giladgray giladgray changed the title Toaster.showOrUpdate() Toaster.show(props, key?) Jan 24, 2018
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 good at a glance.

@giladgray giladgray merged commit 5916ef6 into palantir:develop Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Toaster.showOrUpdate(toast, key)
4 participants