Add Alert Component by aaronpanch · Pull Request #162 · reactstrap/reactstrap

aaronpanch · 2016-09-28T05:26:53Z

Hello! I was working on a small project using reactstrap, made an alert, and thought it would be cool to clean things up and make a PR. In summary, this simply adds a Bootstrap 4 alert, and a documentation page with some examples.

I would like to note a few things for discussion, in addition to any comments or suggestions you might have!

I've added a peer dependency on react-addons-css-transition-group. It wasn't my favorite thing to do; however, it made implementing the dismissible fade-out easier.
I didn't know the order for the documentation pages, so I've added it to the end.
Should there be a link to the Bootstrap docs describing how to use .alert-heading and .alert-link? I only ask to maintain some feature parity with Bootstrap 4.
In issue Component: Alerts #6, there props outlined include 'isOpen' and 'toggle', but those didn't seem to match the bootstrap behavior (from my understanding). It would be cool to add though...apologies for not asking before submitting a PR--it's easy to change and modify!
The accessibility attributes are also included.

Thanks!

Adds a component that renders a Bootstrap 4 alert. Supports dismissing alerts and animated fade out.

eddywashere · 2016-09-28T06:02:48Z

+  dismissible: false
+}
+
+class AlertWrapper extends React.Component {


This should be in it's own file

it might just be one component if you move the ReactCSSTransitionGroup up to Alert. Also, you could pass tag through as the component prop of ReactCSSTransitionGroup.

Yeah, I totally agree--especially if we support a fade-in animation and do the single child thing, there isn't really a need for a separate wrapping component.

eddywashere · 2016-09-28T06:03:55Z

+
+    if (dismissible) {
+      return (
+        <ReactCSSTransitionGroup


Consider using the Render a single child method to avoid the extra element created by ReactCSSTransitionGroup. I've never done this before but it seems like just the right thing to use.

I haven't either, but it seems cool!

eddywashere · 2016-09-28T06:09:20Z

+  }
+
+  onDismiss = () => {
+    this.setState({ visible: false });


I don't think this needs to be an uncontrolled component. Users should control state. I think this could accept the isOpen prop for toggling visibility and use that logic with single child method to control how it's shown. onDismiss would be replaced by the toggle callback.

I agree with the notion that this doesn't need to be an uncontrolled component but for simplicity perhaps theres another way to achieve the same goal? Which do you prefer: someone rendering an long-lasting alert and toggling its visibility or mounting and un-mounting the component?

It could be a controlled and uncontrolled component. But I think that leads to inconsistency (updating a prop vs updating via setState) and extra docs. Preferring props over internal state is a general rule for this library. I built component-template with this sort of thing in mind. Someone could build reactstrap-controlled-components, which wrap these components and adds a controlled version of Alert. It's not the most convenient thing to do but it definitely enables more prescriptive ways of doing things. Even though that prescription is simplicity.

Yeah, I hear ya. In terms of development, a controlled component is real simple so I'll go with that. Are you using the isOpen and toggle terminology to be consistent throughout the library? I suppose isVisible matches what's going on semantically but comes at a cost of inconsistency.

Also, to clarify, I wasn't suggesting a mix of controlled and uncontrolled, rather an approach like a dropdown where it internally controlled its own state. All in all, a controlled component is in the works.

The Dropdown examples do advocate set state but that's in the example files vs the src. The only things I see that use state in the src files are Fade and TabContent. I think isOpen is not the best term but it's not way off from describing this.

I'm cool with isOpen. The fact that people can probably guess most of the property names in this library is awesome.

eddywashere · 2016-09-28T06:10:52Z

+        },
+        {
+          name: 'Alert',
+          to: '/components/alert/'


I would update title and url to be pluralized to closer match v4 docs.

Woops...Good call

eddywashere · 2016-09-28T06:18:26Z

+};
+
+const defaultProps = {
+  color: 'warning',


can you change this to success for default

eddywashere · 2016-09-28T06:30:28Z

+            leave: 'fade',
+            leaveActive: 'out'
+          }}
+          transitionEnter={false}


I think it should be possible to transition on enter, not sure what that involves. Any reason not to include it?

That's a great suggestion. I only omitted it for attempted parity with Bootstrap, but it's an easy 2-liner addition. I suppose we should also support the transitionAppear for initial mounting too.

eddywashere · 2016-09-28T06:30:33Z

+            leaveActive: 'out'
+          }}
+          transitionEnter={false}
+          transitionLeaveTimeout={300}>


I would make this configurable for leave and onenter similar to delay in this example.

See commit 462f6cd. That adds the timeout props to Alert and 1dc4e6d documents it. Setting each to 0 will cancel the respective animation. Is that cool?

Awesome! That works for me.

eddywashere · 2016-09-28T06:54:08Z

Did you look at using the Fade component? It might help avoid the peer dependency. I think I avoided ReactCSSTransitionGroup because I didn't have as much control as I did with ReactTransitionGroup. There's definitely room for improvement in Fade. Thanks for working on this!

aaronpanch · 2016-09-28T16:49:40Z

@eddywashere Thanks for the comments!

One question about procedure: for the changes, should I add separate commits (and leave them separate), or squash the changes into the originals, or separate and squash later (when merging)?

Also, I did take a quick look at the Fade component but it didn't seem to support a transition out (yet), and looked similar to the CSS transition group. I can take a deeper look today.

eddywashere · 2016-09-28T19:34:10Z

Don't worry about squashing things. I prefer detailed commits. I squash and merge via github once it's all good to go. I'll need some time to better describe the limitations I thought I found in css transition group. But feel free to look into it as well. I think it was regarding unique class names on transition life cycle events, maybe something specific to "while transitioning". which bootstrap applies classes to.

eddywashere · 2016-09-29T04:07:56Z

+  return (
+    <ReactCSSTransitionGroup
+      component={FirstChild}
+      transitionName={{


experimented with this, the following example seems to apply the right classes and works with the default opacity animation of .15s.

<ReactCSSTransitionGroup component={FirstChild} transitionName={{ enter: 'fade', enterActive: 'in', leave: 'fade', leaveActive: 'out', appear: 'fade', appearActive: 'in' }} transitionEnterTimeout={150} transitionLeaveTimeout={150} > {isOpen ? alert : null} </ReactCSSTransitionGroup>

eddywashere · 2016-09-29T05:01:21Z

Notes in animation docs

When using ReactCSSTransitionGroup, there's no way for your components to be notified when a transition has ended or to perform any more complex logic around animation. If you want more fine-grained control, you can use the lower-level ReactTransitionGroup API which provides the hooks you need to do custom transitions.

That's why I went with it for Fade. Totally forgot I added hooks for onEnter and onLeave, based on the ReactTransitionGroup lifecycle callbacks. I used onLeave to teardown Modal. I created a gist with the changes I explored to Fade to support Alert. It works for Alert but it breaks Modal. I don't know what it would take to update Modal to use it. Check out the changes here. It seems more involved. I'd be ok with you sticking with ReactCSSTransitionGroup for the quick win and spending more time on making Fade.

If you stick with ReactCSSTransitionGroup, just be sure to add it to npm install portion of docs. Going this route, we don't have to introduce the onEnter, onLeave callbacks until Fade is updated. Also free to add AlertHeading and AlertLink 👍 .

Thanks again for working on this. Can't wait to use it! Let me know what you think about direction for this.

aaronpanch · 2016-09-30T17:43:42Z

Sorry for the delay -- I just moved to a new apartment! I agree with the direction: lets stick with ReactCSSTransitionGroup for the easy win. I'll add it to the docs also. I see what you're saying for the Fade component. I think I have some time to look at it. Also, I'm still adding the parameterized transition timeouts. Almost there!

- Use value of zero to not animate

eddywashere · 2016-10-01T18:23:54Z

@aaronpanch no rush, I see commits added after your comment. Could you let me know when this is good to review?

aaronpanch · 2016-10-03T18:40:44Z

@eddywashere I think they're good to review when you have a chance! Thanks for all the feedback!

eddywashere · 2016-10-06T05:23:09Z

thanks @aaronpanch!

…) (#180)

IndifferentDisdain · 2016-10-20T17:16:34Z

I'm getting the following error after this change; any thoughts? It's causing webpack build to fail.

ERROR in .//reactstrap/lib/Alert.js
Module not found: Error: Cannot resolve module 'react-addons-css-transition-group' in N:\workspaces\credentialing\node_modules\reactstrap\lib
@ .//reactstrap/lib/Alert.js 17:37-81

eddywashere · 2016-10-20T17:22:53Z

@IndifferentDisdain A new peer dependency was added. I was considering moving it to dependencies to avoid problems like this. I'm not sure if that would cause more problems.

For now, I would recommend saving this dep to your project.

IndifferentDisdain · 2016-10-20T17:24:22Z

@eddywashere Thanks, I just rtfm'd and noticed the additional dependency. 😸

Aaron Panchal added 2 commits September 27, 2016 22:33

feat(Alert): add alert component

eda6765

Adds a component that renders a Bootstrap 4 alert. Supports dismissing alerts and animated fade out.

docs(Alert): add examples for standard and dismissible alerts

a1a9fa0

aaronpanch force-pushed the addAlerts branch from e87abc2 to a1a9fa0 Compare September 28, 2016 05:33

eddywashere reviewed Sep 28, 2016

View reviewed changes

Rename Alert docs to Alerts

2eda4e6

Aaron Panchal added 2 commits September 28, 2016 14:19

Make success default alert color

40dbc6f

Combine Alert with transition group and use single-child pattern

8ff3e00

eddywashere reviewed Sep 29, 2016

View reviewed changes

Aaron Panchal added 3 commits September 30, 2016 11:05

Add props to Alert for configuring animation timeout

462f6cd

- Use value of zero to not animate

Add react-addons-css-transition-group in npm install command in docs

ecb1daa

Add properties description to Alerts documentation

1dc4e6d

aaronpanch force-pushed the addAlerts branch from 14894b5 to 1dc4e6d Compare September 30, 2016 18:08

eddywashere merged commit 240e776 into reactstrap:master Oct 6, 2016

eddywashere mentioned this pull request Oct 6, 2016

chore(release): adding 3.4.0 #170

Merged

aaronpanch deleted the addAlerts branch October 11, 2016 20:36

eddywashere pushed a commit that referenced this pull request Oct 18, 2016

style(Alert): Fixed 5 ESLint errors in Alert.js and Alert.spec.js (#162…

ad890d7

…) (#180)

Uh oh!

Conversation

aaronpanch commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronpanch Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddywashere Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddywashere commented Sep 28, 2016

Uh oh!

aaronpanch commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddywashere commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddywashere commented Sep 29, 2016

Uh oh!

aaronpanch commented Sep 30, 2016

Uh oh!

eddywashere commented Oct 1, 2016

Uh oh!

aaronpanch commented Oct 3, 2016

Uh oh!

eddywashere commented Oct 6, 2016

Uh oh!

IndifferentDisdain commented Oct 20, 2016

Uh oh!

eddywashere commented Oct 20, 2016

Uh oh!

IndifferentDisdain commented Oct 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aaronpanch Sep 28, 2016 •

edited

Loading

eddywashere Sep 28, 2016 •

edited

Loading

aaronpanch commented Sep 28, 2016 •

edited

Loading