defaultValue check for null #5551
Conversation
|
Thanks @Rudraksha20! Could you also please add a small note to the 1.35 section of CHANGES.md to mention that |
|
Updated CHANGES.md. |
|
@mramato I feel like I remember having a discussion about this before. Did you have a reason for not checking |
|
At one point Cesium wanted to basically pretend So I'm okay with this change, but someone should smokescreen the widgets and some Sandcastle examples before merging this since it could potentially have unintended consequences (though I doubt we were relying on |
@omh1280 could you please do this? |
There was a problem hiding this comment.
I'll do a smokescreen now. Just that one comment! Thanks @Rudraksha20
CHANGES.md
Outdated
| @@ -28,6 +28,7 @@ Change Log | |||
| * Added a Sandcastle demo for setting time with the Clock API [#5457](https://github.com/AnalyticalGraphicsInc/cesium/pull/5457); | |||
| * Added support for `ParticleSystem`s. [#5212](https://github.com/AnalyticalGraphicsInc/cesium/pull/5212) | |||
| * Added `Cesium.Math.randomBetween`. | |||
| * `deafultValue` updated to check for Null. | |||
There was a problem hiding this comment.
And link this PR in CHANGES.md
|
Everything looks good to me. Once CHANGES.md is updated it'll be good. Thanks @Rudraksha20! |
|
@omh1280 updated CHANGES.md. |
|
Looks good! One of the tests is failing (timeout), but it's not failing locally. This makes me think it's a Travis issue, not a code issue. |
|
I think you're right @omh1280. I restarted the travis build to see if it passes. @Rudraksha20 can you please add a unit test? |
|
@hpinkos, added Unit test for defaultValue. |
Specs/Core/defaultValueSpec.js
Outdated
|
|
||
| it('Works with first parameter undefined', function() { | ||
| expect(function(){ | ||
| defaultValue.defaultValue(undefined, 5); |
There was a problem hiding this comment.
This can just become expect(defaultValue(undefined,5).toEqual(5), I think
There was a problem hiding this comment.
@omh1280 yup, updated with the changes.
|
Should be good once this goes green! |
|
Great work @Rudraksha20! I just tweaked the working in |
Worked on issue #5511, defaultValue check for Null.