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.

Sign up for GitHub

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

Jump to bottom

#824 Add ability to disable fade on <Alert /> #1078

Merged
merged 13 commits into from
Jun 25, 2018
Merged

#824 Add ability to disable fade on <Alert /> #1078

merged 13 commits into from
Jun 25, 2018

Conversation

electriccode
Copy link
Contributor

No description provided.

@electriccode electriccode mentioned this pull request Jun 16, 2018
Closed
TheSharpieOne
TheSharpieOne requested changes Jun 18, 2018
View reviewed changes
src/UncontrolledAlert.js Outdated
constructor(props) {
super(props);

this.state = { isOpen: true };
this.toggle = this.toggle.bind(this);
this.fade = this.props.fade;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think any changes are needed to this file, but at the very least, this change does nothing.

@electriccode
Copy link
Contributor Author

@TheSharpieOne done

@electriccode
Copy link
Contributor Author

electriccode commented Jun 23, 2018 •
edited
Loading

@TheSharpieOne in a84e148 inheriting default props from Alert will override isOpen prop here
<Alert isOpen={this.state.isOpen} {...this.props} and thats not the pattern we follow with Uncontrolled Controls elsewhere. Please review, 2b2b9c9 or suggest if there is a better approach.

@TheSharpieOne
Copy link
Member

Yeah, we usually just don't define the props for the uncontrolled components but when I was cleaning it up I thought it would be interesting if we exposed the original props through. I probably should have only done it for prop types and not default props but I didn't give it much thought.

@electriccode
Copy link
Contributor Author

I can add back prop types from Alert component, but this is not observed at other places so lets stick with this for now.

@TheSharpieOne TheSharpieOne merged commit c71f1d4 into reactstrap:master Jun 25, 2018
juanmaguitar added a commit to juanmaguitar/reactstrap that referenced this pull request Jun 25, 2018
@juanmaguitar
Merge branch 'master' of github.com:reactstrap/reactstrap into toolti…
1fe7965 
…p-innerRef

* 'master' of github.com:reactstrap/reactstrap:
  feat(Modal): add ref to Modal (reactstrap#1087)
  feat(alert): add ability to disable fade (reactstrap#1078)
@juanmaguitar juanmaguitar mentioned this pull request Jun 28, 2018
Merged
@nwalters512 nwalters512 mentioned this pull request Feb 4, 2019
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheSharpieOne TheSharpieOne TheSharpieOne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@electriccode @TheSharpieOne