-
Notifications
You must be signed in to change notification settings - Fork 55
feat(dropdown): control highlightedIndex from Dropdown #966
Conversation
} else { | ||
newState['highlightedIndex'] = this.props.resetHighlightedIndex || null | ||
} | ||
} |
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.
Sorry, but I lost there 😴 If you will try to cover this code with UTs you will not survive 🦇
- Can we extract code that calculates only
highlightedIndex
based on changes/props? - Can we dispatch DOM effects in
componentDidUpdate()
? I.e. in a single place based on the component's state.
componentDidUpdate(prevProps, prevState) {
if (!prevState.open && this.state.open) {
if (search) {
this.listRef.current.focus()
}
if (multiple) {
this.triggerRef.current.focus()
}
}
}
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 extracted the code, you can take a look. as for the DOM effects, it's out of scope, but we can create a separate issue. I also want to move the methods around in the Dropdown, since it's kind of chaos there. should be render, then renderWhatevers, then event handlers, then helpers. Or something like that, but grouped better, since it's a 1100 line file.
@@ -244,6 +254,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo | |||
placeholder: PropTypes.string, | |||
renderItem: PropTypes.func, | |||
renderSelectedItem: PropTypes.func, | |||
resetHighlightedIndex: PropTypes.number, |
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 don't understand why we need this prop when we can control the highlightedIndex
directly, can you please clarify?
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.
we need it so that users don't have to make a whole different highlightedIndex
controlling logic when they just want for the Dropdown to always open at a specified index. For example Add People picker in WebClient, that opens always with first item highlighted.
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.
For me it looks like a case with controlled usage, I think that we should not introduce new props that do exactly the same thing.
/cc @kuzhelov
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 totally agree. If you look at Downshift's documentation, for controlled props, they have also two helpers, initial
and default
. initial
works similar to our default
while default
works similarly to this reset
. I personally think both of them are useful, along with the actual controlling prop. so if we can converge to a solution, that'd be great.
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.
followed up with this #970 in which we can discuss more on the subject.
Changed dependencies in
Generated by 🚫 dangerJS |
Codecov Report
@@ Coverage Diff @@
## master #966 +/- ##
==========================================
+ Coverage 81.76% 82.16% +0.39%
==========================================
Files 701 701
Lines 8573 8597 +24
Branches 1245 1251 +6
==========================================
+ Hits 7010 7064 +54
+ Misses 1548 1517 -31
- Partials 15 16 +1
Continue to review full report at Codecov.
|
? (() => { | ||
const newIndex = this.state.highlightedIndex - 1 | ||
return newIndex < 0 ? itemsLength - 1 : newIndex | ||
})() |
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.
What is a actual reason to introduce IIFE there? I also see that it's the same function as on line 1073 🤔
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.
will change it, I had it when this was in the onStateChange.
})() | ||
: 0 | ||
} | ||
return undefined |
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 method returns undefined
while getHighlightedIndexOnClose()
returns null
💭
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.
May be in this case we should return the current 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.
it's like this because when the menu opens by arrow up/down, we want to modify the highlighted index already saved in state. otherwise, it needs to be left alone, so it will be opened at whatever it was set in the close function.
when the menu closes, we want the index to be either selectedItem for single selection, 0 in case boolean prop is passed with true, or null if nothing else, so we make sure the dropdown opens with no highlight.
@@ -617,11 +634,26 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo | |||
|
|||
private handleStateChange = (changes: StateChangeOptions<ShorthandValue>) => { | |||
if (changes.isOpen !== undefined && changes.isOpen !== this.state.open) { | |||
this.trySetStateAndInvokeHandler('onOpenChange', null, { open: changes.isOpen }) | |||
const newState: { open: boolean; highlightedIndex?: number } = { open: changes.isOpen } |
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.
Now we have these methods that are much more readable 👍
Can we simplify more?
const highlightedIndex = changes.isOpen ? this.getHighlightedIndexOnArrowKeyOpen(changes) : this.getHighlightedIndexOnClose(changes)
const newState = { open: changes.isOpen, highlightedIndex }
if (changes.isOpen && !this.props.search) {
this.listRef.current.focus()
}
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 only solution is to have const newState = { open: changes.isOpen, ...{highlightedIndex} }
to make sure that the highlightedIndex is not passed as undefined
. otherwise Downshift will complain that we are transforming a controlled prop to uncontrolled.
should also aim to fix #979 |
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.
some comments, good job 👍
} | ||
|
||
this.trySetState(newState) | ||
_.invoke(this.props, 'onOpenChange', null, { ...this.props, ...newState }) |
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.
logic on lines 651 and 652 was encapsulated in the method that used to be at line 620; can you please use that again?
this.trySetStateAndInvokeHandler('onOpenChange', null, newState)
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, will do
changes: StateChangeOptions<ShorthandValue>, | ||
): number => { | ||
// if open by ArrowUp, index should change by -1. | ||
if (changes.type === Downshift.stateChangeTypes.keyDownArrowUp) { |
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.
we were using switch/case for this type of conditions; let's aim for consistency and do the same here
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.
will change.
return itemsLength - 1 | ||
} | ||
// if open by ArrowDown, index should change by +1. | ||
if (changes.type === Downshift.stateChangeTypes.keyDownArrowDown) { |
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 have been if/else statement anyway as the conditions are independent
const newIndex = this.state.highlightedIndex + 1 | ||
return newIndex >= itemsLength ? 0 : newIndex | ||
} | ||
return 0 |
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.
can we encapsulate logic form lines 1072-1077 (and 1063-1068) in smaller methods? logic is complex and because of branching it becomes hard to read
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.
there are just 4 lines each, and a separate method with some more if-else to change values depending on that stateChangeTypes
... I would rather leave it like this.
return undefined | ||
} | ||
|
||
private getHighlightedIndexOnClose = (): number => { |
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.
NIT: for readability:
private getHighlightedIndexOnClose = (): number => {
const { highlightFirstItemOnOpen, items, multiple, search } = this.props
const { value } = this.state
if (!multiple && !search && value) {
// in single selection, if there is a selected item, highlight it.
return items.indexOf(value)
}
if (highlightFirstItemOnOpen) {
// otherwise, if highlightFirstItemOnOpen prop is provied, highlight first item
return 0
}
// otherwise, highlight no item
return null
}
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, will use that version
1f11461
to
aadf824
Compare
Means to control
highlightedIndex
prop and fixes #903