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

[WIP] Add support for Bootstrap 4.2's Toasts feature (unfinished PR. feedback welcome) #1384

Closed
wants to merge 8 commits into from

Conversation

hcharley
Copy link
Contributor

@hcharley hcharley commented Jan 29, 2019

WIP for issue #1346

Screenshot of progress: https://user-images.githubusercontent.com/1542740/51679897-2eb69900-1fae-11e9-85eb-01a7ba9242de.png

--

  • Bug fix
  • New feature
  • Chore
  • Breaking change
  • There is an open issue which this change addresses
  • I have read the CONTRIBUTING document.
  • My commits follow the Git Commit Guidelines
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

},
};

function Alert(props) {
Copy link
Member

Choose a reason for hiding this comment

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

Change Alert to Toast (here and below)

const classes = mapToCssModules(classNames(
className,
'toast',
body ? 'toast-body' : false,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a thing: .toast.toast-body? I didn't notice any examples where those classes were on the same element.

return (
<WrapTag {...attributes} className={classes}>
{icon && (
<svg
Copy link
Member

Choose a reason for hiding this comment

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

I think this svg was just for the examples in the bootstrap docs. It would be good to allow the user to specific something custom.
Though, I do like the idea of specifying a color to have a square of that color. Maybe allow a component or a string. In the case of a component, the component is used as is. When it is a string, this svg can be used.

@@ -1,3 +1,5 @@
import ToastHeader from "./ToastHeader";
Copy link
Member

Choose a reason for hiding this comment

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

You're importing the ToastHeader? Why?


toggle() {
this.setState({
show: !this.state.show

Choose a reason for hiding this comment

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

better to use prevState here.

@GoPro16
Copy link
Member

GoPro16 commented Mar 1, 2019

I was just looking to add this feature in and found this open PR. I was wondering if we could add in an additional prop such as position with possible values of top, top-right, bottom, bottom-right etc.

Either this can be added or we can do something like the below:

Create a Toaster container that creates toasts:

<Toaster position="top-right">
..Normal Application
</Toaster>

Then export a context and useToaster like the below:

const toast = useToaster();

Then in any underlying component you can call the toaster to create by just using the toast method from the provider.

Wanted to get everyone's thoughts on this and possibly add this in as a feature after this has been merged in.

@samuelcolvin
Copy link

Any progress on this?

Would be wonderful to get this deployed.

@hcharley
Copy link
Contributor Author

@samuelcolvin I haven't had any time to work on this, but anyone is welcome to pick up where I left off. I think I did much of the gruntwork of putting together the right files, but there needs to be much more finessing.

@seansfkelley seansfkelley mentioned this pull request Mar 28, 2019
12 tasks
@seansfkelley
Copy link
Contributor

I just opened #1447 to address the feedback on this PR and hopefully get it over the line.

TheSharpieOne pushed a commit that referenced this pull request Apr 3, 2019
Supersedes and closes #1384. Fixes #1346.

Note that I didn't rewrite @Charlex's commit to adhere to the commit guidelines cause I didn't want to change history and make it look like I wrote that code. Hope that's alright.
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.

None yet

7 participants