Fix issue with "Infinity" value not working #12980
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s "hide the notification"
stacey-gammon
requested review from
jbudz and
pickypg
and removed request for
jbudz
jbudz
approved these changes
Jul 19, 2017
epixa
reviewed
Jul 19, 2017
src/ui/public/notify/notifier.js
Outdated
| @@ -63,7 +63,7 @@ function timerCanceler(notif, cb = _.noop, key) { | |||
| function startNotifTimer(notif, cb) { | |||
| const interval = 1000; | |||
|
|
|||
| if (notif.lifetime === Infinity || notif.lifetime === 0) { | |||
| if (notif.lifetime === 'Infinity' || notif.lifetime === Infinity || notif.lifetime === 0) { | |||
There was a problem hiding this comment.
This doesn't seem like the right place to handle this case. The reason the string 'Infinity' is intended to be supported in this PR is because of a use case with the advanced settings UI itself, so it seems like this value should be serialized there long before it gets passed to the notifier.
There was a problem hiding this comment.
I can appreciate that, but I think the benefit of doing it here is that every place that uses the advanced config options can ignore the output and hand it off here. Unless we want to provide a getter on the config object that can parse this for every caller.
pickypg
approved these changes
Jul 19, 2017
| @@ -125,6 +125,8 @@ will set the initial value if one is not already set.`); | |||
| const currentValue = config.isDefault(key) ? defaultValue : userValue; | |||
| if (type === 'json') { | |||
| return JSON.parse(currentValue); | |||
| } else if (type === 'number') { | |||
| return parseFloat(currentValue); | |||
There was a problem hiding this comment.
👍 This was easier than I expected. Kudos on finding this part.
LeeDr
added
bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Typing in "Infinity" into an advanced setting makes it a string, not the special javascript "Infinity" value, so look for both when determine lifetime notification. Also, treat negative numbers as "hide this notification".
Fixes #7565
Note to test this, you have to refresh the page after changing the advanced setting, to get the new notification value picked up.
cc @LeeDr