Add type check to PropType on OneOf #2653
Conversation
Hypnosphi
added
addon: info
bug
patch:yes
Codecov Report
@@ Coverage Diff @@
## master #2653 +/- ##
==========================================
- Coverage 37.38% 37.38% -0.01%
==========================================
Files 426 426
Lines 9150 9153 +3
Branches 869 882 +13
==========================================
+ Hits 3421 3422 +1
+ Misses 5226 5182 -44
- Partials 503 549 +46
Continue to review full report at Codecov.
|
| @@ -1,7 +1,13 @@ | |||
| import React from 'react'; | |||
| import { TypeInfo } from './proptypes'; | |||
|
|
|||
| const OneOf = ({ propType }) => <span>{propType.value.map(({ value }) => value).join(' | ')}</span>; | |||
| const isObject = propType => typeof propType.value === 'object'; | |||
There was a problem hiding this comment.
Should it rather check that propType.value is an array, as we're using an array method?
There was a problem hiding this comment.
I'm not sure why the typeof propType.value returns object when array. Better to change to Array.isArray()
There was a problem hiding this comment.
why the typeof propType.value returns object when array
Because array is an object =)
|
Please add a prop with |
|
@Hypnosphi ? |
|
Yes, that would do it |
|
You might need to update snapshots of example stories by running |
|
@Hypnosphi snapshots updated |
| @@ -19,7 +19,7 @@ exports[`Storyshots Addons|Knobs.withKnobs tweaks static values 1`] = ` | |||
| My name is Storyteller, I'm 70 years old, and my favorite fruit is apple. | |||
| </p> | |||
| <p> | |||
| My birthday is: January 20, 2017 | |||
| My birthday is: January 19, 2017 | |||
There was a problem hiding this comment.
@tmeasday looks like your fix don't work in some cases
There was a problem hiding this comment.
@abrahamgnz just for reference, which timezone are you in?
There was a problem hiding this comment.
CST @Hypnosphi
There was a problem hiding this comment.
@abrahamgnz it would be interesting to know what the following output in the node REPL for you:
new Date('Jan 20 2017 GMT+0').toLocaleDateString('en-US')There was a problem hiding this comment.
+new Date('Jan 20 2017 GMT+0')
1484870400000
@tmeasday
I got the same
There was a problem hiding this comment.
@tmeasday looks like we have to add timeZone: 'UTC' to dateOptions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
en-US is a format not a timezone
the default is the runtime's default time zone
The midnight in London is definitely the same day in Netherlands, Russia, Israel, India, and Australia, that's why it worked for most of the core contributors =)
There was a problem hiding this comment.
I can't believe no-one in the US ran into this yet... not even the CI servers!
I can't really explain why it was printing out the 19th in the first place for me then (given new Date('Jan 20 2017') was midnight 20 Jan in my TZ). But let's lock it down.
There was a problem hiding this comment.
This was probably fixed in #2861
@abrahamgnz can you please try to update snapshots once again?
# Conflicts: # addons/info/src/components/types/OneOf.js
Add type check to PropType on OneOf
Hypnosphi
added
patch:done
Issue: #2652
What I did
Added a type check to PropType prop on OneOf component.
How to test
Add a story with at component that has a oneOf prop with an evaluation in the propType declaration.
See issue for further information.
Is this testable with jest or storyshots?
No
Does this need a new example in the kitchen sink apps?
No
Does this need an update to the documentation?
No