Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd warning when user use Object for selectedItem #264
Conversation
|
Thanks! A few requests. |
| @@ -67,7 +68,12 @@ class Downshift extends Component { | |||
| defaultIsOpen: false, | |||
| getA11yStatusMessage, | |||
| id: generateId('downshift'), | |||
| itemToString: i => (i == null ? '' : String(i)), | |||
| itemToString: i => { | |||
| if (isPlainObject(i)) { | |||
kentcdodds
Nov 27, 2017
Member
I'd prefer this to only happen in dev. I think the build is wired up to handle process.env.NODE_ENV properly. Could you make this: if (process.env.NODE_ENV !== 'production' && isPlainObject(i)) {
I'd prefer this to only happen in dev. I think the build is wired up to handle process.env.NODE_ENV properly. Could you make this: if (process.env.NODE_ENV !== 'production' && isPlainObject(i)) {
philipyoungg
Nov 27, 2017
Author
Contributor
Would process.env.NODE_ENV === 'development' more prefereable in this instance?
Would process.env.NODE_ENV === 'development' more prefereable in this instance?
| itemToString: i => (i == null ? '' : String(i)), | ||
| itemToString: i => { | ||
| if (isPlainObject(i)) { | ||
| console.warn('you passed an Object as the value of `selectedItem`. Please refer to `itemToString` API documentation on Downshift\'s repository.') |
kentcdodds
Nov 27, 2017
Member
I think prettier should have reformatted this automatically when you committed. Did you npm install?
I think prettier should have reformatted this automatically when you committed. Did you npm install?
| @@ -67,7 +68,12 @@ class Downshift extends Component { | |||
| defaultIsOpen: false, | |||
| getA11yStatusMessage, | |||
| id: generateId('downshift'), | |||
| itemToString: i => (i == null ? '' : String(i)), | |||
| itemToString: i => { | |||
kentcdodds
Nov 27, 2017
Member
I'd like to have a test for this. Could you add one in downshift.misc.js? Just do jest.spyOn(console, 'warn') on the first line of the test, then console.warn.mockRestore() on the last line to clean up. You could even use a snapshot if you want: expect(console.warn).toHaveBeenCalledTimes(1) and then expect(console.warn.mock.calls).toMatchSnapshot()
I'd like to have a test for this. Could you add one in downshift.misc.js? Just do jest.spyOn(console, 'warn') on the first line of the test, then console.warn.mockRestore() on the last line to clean up. You could even use a snapshot if you want: expect(console.warn).toHaveBeenCalledTimes(1) and then expect(console.warn.mock.calls).toMatchSnapshot()
philipyoungg
Nov 27, 2017
Author
Contributor
Sure!
Sure!
update
|
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 299 304 +5
Branches 72 73 +1
=====================================
+ Hits 299 304 +5
Continue to review full report at Codecov.
|
|
I made a few updates. This is a great addition. Thank you @philipyoungg |
What: Add
console.warnwhen user passed Object toselectedItemWhy: To improve DX—to minimize a case when user get
[object Object]as an input value. Almost refactored my whole codebase. I thought I can't useObjectas a selected value. Thankfully I founditemToStringAPI that accept a function with currentselectedItempassed on the argument.How: Two things:
isPlainObjectfunction that check if theselectedItemis Object literal, anditemToString's defaultPropsChecklist: