feat(dropdown): control highlightedIndex from Dropdown

[silviuaavram]

Means to control highlightedIndex prop and fixes #903

[layershifter]

Sorry, but I lost there 😴 If you will try to cover this code with UTs you will not survive 🦇

1. Can we extract code that calculates only highlightedIndex based on changes/props?
2. 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()
    }
  }
}

[silviuaavram]

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.

[layershifter]

I don't understand why we need this prop when we can control the highlightedIndex directly, can you please clarify?

[silviuaavram]

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.

[layershifter]

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

[silviuaavram]

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.

[silviuaavram]

followed up with this #970 in which we can discuss more on the subject.

[DustyTheBot]

	Warnings
⚠️	Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies in packages/react/package.json

package	before	after
downshift	^3.2.0	^3.2.6

Generated by 🚫 dangerJS

[codecov]

Codecov Report

Merging #966 into master will increase coverage by 0.39%.
The diff coverage is 83.33%.

@@            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

Impacted Files	Coverage Δ
...ackages/react/src/components/Dropdown/Dropdown.tsx	54.14% <83.33%> (+11.24%)	⬆️
...ges/react/src/components/Dropdown/DropdownItem.tsx	94.44% <0%> (+16.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a4d22...7b5b2ca. Read the comment docs.

[layershifter]

What is a actual reason to introduce IIFE there? I also see that it's the same function as on line 1073 🤔

[silviuaavram]

will change it, I had it when this was in the onStateChange.

[layershifter]

This method returns undefined while getHighlightedIndexOnClose() returns null 💭

[layershifter]

May be in this case we should return the current value?

[silviuaavram]

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.

[layershifter]

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()
}

[silviuaavram]

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.

[silviuaavram]

should also aim to fix #979

[bmdalex]

some comments, good job 👍

[bmdalex]

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)

[silviuaavram]

thanks, will do

[bmdalex]

we were using switch/case for this type of conditions; let's aim for consistency and do the same here

[silviuaavram]

will change.

[bmdalex]

this should have been if/else statement anyway as the conditions are independent

[bmdalex]

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

[silviuaavram]

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.

[bmdalex]

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
}

[silviuaavram]

thanks, will use that version