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

Time To Live Indicator for toasts #189

Merged
merged 5 commits into from
Oct 16, 2014
Merged

Conversation

tradiff
Copy link
Contributor

@tradiff tradiff commented Oct 7, 2014

Add an optional progress bar to toasts to indicate time to live. This is related to Issue 188

@TimFerrell
Copy link
Member

Thanks for the PR.
I'll be able to review this later in detail. Something sticks out to me: what's the reason for the 1px change on the media queries?

@tradiff
Copy link
Contributor Author

tradiff commented Oct 7, 2014

I assume you're talking about the toastr.scss file. Toastr.scss seemed to be a direct copy of toastr.css (which itself is generated from toastr.less), but it had those few differences. I assumed the differences were not intentional, so when I copied toastr.css to toastr.scss, I allowed the differences to be fixed.

If for some reason you want the scss media queries to be different from the less and css, or if you think it doesn't belong in this pull request, I can change it back.

@tradiff
Copy link
Contributor Author

tradiff commented Oct 11, 2014

Has this been reviewed yet?

@TimFerrell
Copy link
Member

Been a busy week - will try to complete review this weekend.

@TimFerrell
Copy link
Member

No issues here. Functional tests pass along with Travis tests.

@johnpapa - do you have any comments prior to merge?

@@ -211,9 +212,13 @@

$container = getContainer(options, true);
var intervalId = null,
progressIntervalId = null,
Copy link
Member

Choose a reason for hiding this comment

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

if we need these settings to stay at this closure level and they are all related to the progressBar, I suggest they be made in a pbOptions object. Self documenting. Otherwise it is confusing as to what hideTimeout and the others are used for.

pbOptions = { }

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Good call. Agreed. @travistx - can you make the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnpapa I just want to make sure you are aware that the variables are not options. They are internal variables. Do you still want them in their own object? Or perhaps renamed more appropriately?

currentTimeOut = The total seconds before the toastr will hide itself. This will always be equal to options.timeOut or options.extendedTimeOut.

hideTime = The eta in which the toastr will be hiding itself.

progressIntervalId = The id for the setInterval which updates the progress bar width. This is stored so that it can be cleared when the toaster is hidden.

Copy link
Member

Choose a reason for hiding this comment

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

the spirit is to name them in a way that makes them easy to read and understand what they are for. right now they are not clear. so an object literal or better naming is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Please see the latest commit.

@TimFerrell
Copy link
Member

Unit tests pass both locally and on Travis. Functional tests also pass when tested locally. Good to go from my end.

@TimFerrell
Copy link
Member

@johnpapa - any issues with merging this?

johnpapa added a commit that referenced this pull request Oct 16, 2014
Time To Live Indicator for toasts
@johnpapa johnpapa merged commit e027e1e into CodeSeven:master Oct 16, 2014
@johnpapa
Copy link
Member

thanks for the contribution

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