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

expanded alert component to handle auto hide #14

Merged
merged 6 commits into from
Dec 4, 2018

Conversation

joefitter
Copy link
Contributor

No description provided.

Copy link
Contributor

@wheelsandcogs wheelsandcogs left a comment

Choose a reason for hiding this comment

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

Looks ok.

@joefitter joefitter changed the title expanded alert component to handle auto hide WIP expanded alert component to handle auto hide Dec 3, 2018
@joefitter
Copy link
Contributor Author

WIPing this as need a way to pass props to the alert snippet

@joefitter joefitter changed the title WIP expanded alert component to handle auto hide expanded alert component to handle auto hide Dec 3, 2018
@joefitter joefitter force-pushed the bugfix/notifications-component branch from c563c9b to 902b488 Compare December 4, 2018 09:10
@joefitter joefitter force-pushed the bugfix/notifications-component branch from 902b488 to 7d06043 Compare December 4, 2018 09:12
timer() {
if (this.props.timeout && this.props.message) {
notificationTimeout = setTimeout(this.props.hideNotification, this.props.timeout);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to clear the timeout somewhere around here too.

In the case where a notification fires, and then 4 seconds later another one fires, the latter would only be displayed for 1 second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good point

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also move it outside the condition, to cover even more extreme edge cases where the first notification was se to time out, but the second was not.

@@ -1,9 +1,9 @@
import React, { Fragment } from 'react';
import { Alert } from '../';
import { Notifiction } from '../';
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's mildly alarming that this passed CI.

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 guessing it must either be undefined by default (which is CRAZY) or more likely: rollup sucks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another reason we should get rid of the build - no errors are currently thrown on compile

@lennym lennym merged commit 4f31ab7 into master Dec 4, 2018
@lennym lennym deleted the bugfix/notifications-component branch December 4, 2018 09:40
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.

3 participants