-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Reset selected item only when list values change #33
Conversation
.gitignore
Outdated
@@ -1,3 +1,4 @@ | |||
build | |||
node_modules | |||
yarn.lock | |||
.vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in your own global gitignore, not here. https://gist.github.com/subfuzion/db7f57fff2fb6998a16c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for mentioning this! Fixed
package.json
Outdated
"name": "Megrez Lu", | ||
"email": "[email protected]" | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contributors
field is not used by npm or any tool and it's a chore to maintain. Your contribution will be reflected in the commit history and https://github.com/vadimdemedes/ink-select-input/graphs/contributors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Removed
src/SelectInput.tsx
Outdated
@@ -82,7 +83,7 @@ function SelectInput<V>({ | |||
const previousItems = useRef<Array<Item<V>>>(items); | |||
|
|||
useEffect(() => { | |||
if (!isEqual(previousItems.current, items)) { | |||
if (!isEqual(_map(previousItems.current, 'value'), _map(items, 'value'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use native .map()
function over the one in lodash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. But there seems to be a style issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with prettier
package.json
Outdated
@@ -7,7 +7,7 @@ | |||
"author": { | |||
"name": "Vadim Demedes", | |||
"email": "[email protected]", | |||
"url": "github.com/vadimdemedes" | |||
"url": "https://github.com/vadimdemedes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, could you revert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
.gitignore
Outdated
@@ -1,3 +1,3 @@ | |||
build | |||
node_modules | |||
yarn.lock | |||
yarn.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, could you revert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
Minor feedback, but looks good overall, thanks! |
Thanks! |
Released in 4.2.0. |
* upstream/master: 5.0.0 Migrate to ESM and Ink 4 (vadimdemedes#46) 4.2.2 Refresh dependencies Fix stuck selection when `initialIndex` is larger than `limit` (vadimdemedes#44) 4.2.1 Update peer dependencies to allow React 17.x (vadimdemedes#37) Fix old version link in readme 4.2.0 Reset selected item only when list values change (vadimdemedes#33)
Fix issue #22