feat(dropdown): control highlightedIndex from Dropdown #966

silviuaavram · 2019-02-26T09:15:38Z

Means to control highlightedIndex prop and fixes #903

layershifter · 2019-02-26T09:26:51Z

packages/react/src/components/Dropdown/Dropdown.tsx

+        } else {
+          newState['highlightedIndex'] = this.props.resetHighlightedIndex || null
+        }
+      }


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

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 · 2019-02-26T09:29:02Z

packages/react/src/components/Dropdown/Dropdown.tsx

@@ -244,6 +254,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo
    placeholder: PropTypes.string,
    renderItem: PropTypes.func,
    renderSelectedItem: PropTypes.func,
+    resetHighlightedIndex: PropTypes.number,


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

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.

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

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.

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

DustyTheBot · 2019-02-26T13:04:02Z

	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 · 2019-02-26T13:04:08Z

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 · 2019-02-26T13:56:25Z

packages/react/src/components/Dropdown/Dropdown.tsx

+        ? (() => {
+            const newIndex = this.state.highlightedIndex - 1
+            return newIndex < 0 ? itemsLength - 1 : newIndex
+          })()


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

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

layershifter · 2019-02-26T14:00:56Z

packages/react/src/components/Dropdown/Dropdown.tsx

+          })()
+        : 0
+    }
+    return undefined


This method returns undefined while getHighlightedIndexOnClose() returns null 💭

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

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 · 2019-02-26T14:08:12Z

packages/react/src/components/Dropdown/Dropdown.tsx

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


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

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 · 2019-02-27T11:16:32Z

should also aim to fix #979

bmdalex

some comments, good job 👍

bmdalex · 2019-02-27T16:24:16Z

packages/react/src/components/Dropdown/Dropdown.tsx

+      }
+
+      this.trySetState(newState)
+      _.invoke(this.props, 'onOpenChange', null, { ...this.props, ...newState })


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)

thanks, will do

bmdalex · 2019-02-27T16:26:05Z

packages/react/src/components/Dropdown/Dropdown.tsx

+    changes: StateChangeOptions<ShorthandValue>,
+  ): number => {
+    // if open by ArrowUp, index should change by -1.
+    if (changes.type === Downshift.stateChangeTypes.keyDownArrowUp) {


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

will change.

bmdalex · 2019-02-27T16:26:37Z

packages/react/src/components/Dropdown/Dropdown.tsx

+      return itemsLength - 1
+    }
+    // if open by ArrowDown, index should change by +1.
+    if (changes.type === Downshift.stateChangeTypes.keyDownArrowDown) {


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

bmdalex · 2019-02-27T16:28:11Z

packages/react/src/components/Dropdown/Dropdown.tsx

+        const newIndex = this.state.highlightedIndex + 1
+        return newIndex >= itemsLength ? 0 : newIndex
+      }
+      return 0


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 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 · 2019-02-27T16:38:41Z

packages/react/src/components/Dropdown/Dropdown.tsx

+    return undefined
+  }
+
+  private getHighlightedIndexOnClose = (): number => {


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 }

thanks, will use that version

packages/react/src/components/Dropdown/Dropdown.tsx

silviuaavram requested review from dvdzkwsk, kuzhelov, layershifter, levithomason, miroslavstastny and mnajdova as code owners February 26, 2019 09:15

layershifter reviewed Feb 26, 2019

View reviewed changes

bmdalex approved these changes Feb 27, 2019

View reviewed changes

silviuaavram added the 🚀 ready for review label Feb 28, 2019

Silviu Avram added 14 commits March 1, 2019 15:33

updated Downshift version

3f0c42f

controlled the highlightedIndex

ac621fc

fixed a compile issue

55c1b08

fixed wrapping index on key downs

194d4e3

increased downshift version

274f419

calculates new index from filtered items

72ea9f8

fixed a unit test

be3eb22

changed the api to provide a boolean value for first item highlight

62a260a

added a new line

f29a3f0

extracted highlightedIndex logic in separate methods

e1512ef

some changes in the methods

90d17f9

adressed comments from Alex

a311be1

fixed some things in handleStateChange

1305cbd

added some unit tests

aadf824

silviuaavram force-pushed the feat/control-highlighted-index branch from 1f11461 to aadf824 Compare March 1, 2019 14:33

Silviu Avram and others added 4 commits March 5, 2019 12:26

fixed a rollup issue

4b49067

added Delete to rollup

887be97

Merge branch 'master' into feat/control-highlighted-index

b72d01f

Merge branch 'master' into feat/control-highlighted-index

73cea03

layershifter approved these changes Mar 21, 2019

View reviewed changes

jurokapsiar and others added 2 commits March 21, 2019 18:36

Merge branch 'master' into feat/control-highlighted-index

7d1f200

changelog

7b5b2ca

jurokapsiar merged commit 6aa648a into master Mar 21, 2019

delete-merged-branch bot deleted the feat/control-highlighted-index branch March 21, 2019 18:43

bmdalex mentioned this pull request Mar 25, 2019

Dropdown does not select items by default when opened #979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dropdown): control highlightedIndex from Dropdown #966

feat(dropdown): control highlightedIndex from Dropdown #966

silviuaavram commented Feb 26, 2019 •

edited by layershifter

layershifter Feb 26, 2019 •

edited

silviuaavram Feb 26, 2019

layershifter Feb 26, 2019

silviuaavram Feb 26, 2019

layershifter Feb 26, 2019

silviuaavram Feb 26, 2019

silviuaavram Feb 26, 2019

DustyTheBot commented Feb 26, 2019 •

edited

codecov bot commented Feb 26, 2019 •

edited

layershifter Feb 26, 2019

silviuaavram Feb 26, 2019

layershifter Feb 26, 2019

layershifter Feb 26, 2019

silviuaavram Feb 26, 2019

layershifter Feb 26, 2019

silviuaavram Feb 26, 2019 •

edited

silviuaavram commented Feb 27, 2019

bmdalex left a comment

bmdalex Feb 27, 2019

silviuaavram Feb 28, 2019

bmdalex Feb 27, 2019

silviuaavram Feb 28, 2019

bmdalex Feb 27, 2019

bmdalex Feb 27, 2019

silviuaavram Feb 28, 2019

bmdalex Feb 27, 2019

silviuaavram Feb 28, 2019

feat(dropdown): control highlightedIndex from Dropdown #966

feat(dropdown): control highlightedIndex from Dropdown #966

Conversation

silviuaavram commented Feb 26, 2019 • edited by layershifter

layershifter Feb 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustyTheBot commented Feb 26, 2019 • edited

codecov bot commented Feb 26, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silviuaavram Feb 26, 2019 • edited

Choose a reason for hiding this comment

silviuaavram commented Feb 27, 2019

bmdalex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silviuaavram commented Feb 26, 2019 •

edited by layershifter

layershifter Feb 26, 2019 •

edited

DustyTheBot commented Feb 26, 2019 •

edited

codecov bot commented Feb 26, 2019 •

edited

silviuaavram Feb 26, 2019 •

edited