fix: Fix TimePicker filter behavior. Misc ComboBox Fixes #1139

brandonlenz · 2021-04-15T03:05:30Z

Summary

The MVP TimePicker filtering behavior was identical to our ComboBox implementation, since it was just wrapping the ComboBox.

USWDS made some changes to filtering to the ComboBox (see #1108). This PR also brings those enhancements to our reactified library.

There was also some existing behavior of our combobox that did not match the USWDS combobox (whether that was always the case, or due to the USWDS changes since our original implementation, I don't know, but I fixed as many as I found here along the way).

Previous ComboBox behavior can be tested at our hosted storybook: https://trussworks.github.io/react-uswds/?path=/story/components-form-controls-combo-box--default-combo-box-with-prop-options

Current USWDS ComboBox behavior: https://designsystem.digital.gov/components/combo-box/

Previous TimePicker behavior can be tested at our hosted storybook: https://trussworks.github.io/react-uswds/?path=/story/components-form-controls-time-picker--complete-time-picker

Current USWDS TimePicker behavior: https://designsystem.digital.gov/components/time-picker/

The following issues were found & resolved (and tests added to hopefully cement the fixes since they're not all obvious):

Note: The videos show the following:

USWDS Model Behavior Current Behavior Fixed Behavior (this PR)

Pressing enter on the input without an exact match, but with a previously selected option should revert to the selected option.

Enter.without.exact.match.reverts.to.prev.selection.mov

Typing into the input to filter options, then Blurring without selecting should reset the filter (previously it would keep the filter until the input value was changed)

Reset.filters.on.blur.mov

Selecting an option should give that option the selected style in the dropdown, even if the input filter allows more than one option to be seen.

Selected.option.maintains.selected.styling.if.filtered.mov

When no option is selected, but the dropdown is open, the first option should have the focused style applied (but not actually be focused)

First.option.has.focused.style.mov

When the ComboBox is opened and scrolled, then closed, and reopened it should either be at the top, OR if an item is selected, that item should be scrolled into view.

Should.auto.scroll.to.top.or.to.selected.item.mov

When an option is selected, but the filter is changed such that the selected option is not visible, the first partial match should be highlighted with the focused style (but not actually be focused)

First.option.should.be.focused.if.filtering.if.the.selected.option.is.not.visible.mov

TimePicker should not filter via hiding options, but should move the focus styled element to the first match (and scroll to said match)

time.picker.should.not.filter.options.mov

TimePicker should pass custom filtering logic into combobox to generate regex to find options. This allows 5:3p when typed to match 5:30pm:

Time.picker.filtering.works.properly.mov

Related Issues or PRs

closes: #1108

How To Test

The unit tests I added help to cover the scenarios I fixed.

To compare to our existing storybook or the USWDS implementation, use the deployed storybook or checkout this branch and run storybook locally.

@christopherhuii I would love your help in testing this in a project since I want to make sure I didn't break the blurring behavior and focus trap fixes you made recently (I did some testing locally, but I'm not sure I covered everything)

- Verify first element has focused class - Mock scrollIntoView to keep tests from failing

- Use regex pattern and CustomizableFilter structure - Unfilter items on Blur: Either an item is selected, and all options should be shown when the list is expanded again, or no option is selected, the field is cleared, and all items should be shown. - Remove almost unused 'filter' prop from state - Update TimePicker to use the custom filtering logic

- Allow TimePickerTests to run (mock scroll function) - Track "closest match" for styling first matching item when filtering the list is disabled - hide clear button when tpying in combobox - Update old bug test to succesfully check style

brandonlenz · 2021-04-15T03:47:14Z

src/components/forms/ComboBox/utils.ts

+ * @param {string} query The value to use in the regular expression
+ * @param {object} extras An object of regular expressions to replace and filter the query
+ */
+export const generateDynamicRegExp = (


FYI for reviewers, this is from USWDS

brandonlenz · 2021-04-15T03:50:07Z

Looks like the Happo Diffs are from

brandonlenz · 2021-04-19T19:15:46Z

After an embarrassing demo this morning where I had inadvertently broken one of the first things I fixed in this PR, I made some changes.

In addition to fixing the auto-scroll behavior again, I removed the extra state prop I was using to track the "closest match". This was so similar to focusedOption that I realized it was not only confusing to track both, but also necessary.

Then I added in a few more tests and fixed a test that was previously skipped due to a bug that this refactor happened to fix along the way 🥳 🎉

Thanks Demo effect!

…trussworks/react-uswds into bl-time-picker-combo-box-filtering-1108

brandonlenz · 2021-04-20T23:42:49Z

Added screen recordings to the description to help illustrate the other misc. fixes I made 😅

haworku

Great work. Whew. The videos helped a lot to understand the expected behavior - really good example of how to make code review easier. I'm excited for some of the new behavior added.

Some of the issues with focus I think could be regressions (specifically the ones about closing/blurring and then coming back and reopening and certain items being focused or highlighted). That logic gets very nuanced particularly with filtering that's in progress. Either way, its good to have more tests around this and remove some of our 🐛 fix TODOs 😆.

Agree that we should have easi or @christopherhuii test this in their app before merging anything to main.

haworku · 2021-05-06T19:57:50Z

src/components/forms/ComboBox/ComboBox.test.tsx

+      'usa-combo-box__list-option--focused usa-combo-box__list-option--selected'
+    )
+    expect(scrollFunction).toHaveBeenCalledTimes(1)
+  })


haworku · 2021-05-06T20:05:08Z

src/components/forms/ComboBox/ComboBox.test.tsx

@@ -873,6 +1018,9 @@ describe('ComboBox component', () => {

      expect(getByTestId('combo-box-clear-button')).not.toBeVisible()
      expect(getByTestId('combo-box-option-list').children.length).toEqual(1)
+
+      fireEvent.blur(input)
+      expect


why call expect with no value? I've never seen this pattern now I'm curious

uuuuuuh 😅

This looks like I started writing expects for this tests, then saw a squirrel, then came back and committed. Nice catch! I'm surprised it's valid code tbh 😆

I'll fix this

yeah it was valid code and I was like -- hmm does this do something I don't know lol

Fixed in these two commits

haworku · 2021-05-06T20:20:05Z

src/components/forms/ComboBox/ComboBox.tsx


  const containerRef = useRef<HTMLDivElement>(null)
-  const itemRef = useRef<HTMLLIElement>(null)
+  const focusedItemRef = useRef<HTMLLIElement>(null)


This renaming is clarifying ⭐

brandonlenz added 17 commits April 13, 2021 15:38

Add disableFiltering prop

502cf55

Focus the selected opion on arrow down

ac45cb3

Scroll selected option into view on list open

73b116c

Just set the options list to all options instead of filtering on ''

f3adf24

Test Updates:

c98aaa1

- Verify first element has focused class - Mock scrollIntoView to keep tests from failing

When scrolling into view, scroll so item is at bottom

a76d7a0

Match casing to USWDS

af9bd74

Fix several differences with USWDS Combobox

eef8bfa

- Allow TimePickerTests to run (mock scroll function) - Track "closest match" for styling first matching item when filtering the list is disabled - hide clear button when tpying in combobox - Update old bug test to succesfully check style

Remove console.logs

02e1310

Add TimePicker tests. Make ComboBox closestMatch always have a value

a3a6797

Add test to verify disableFiltering does not hide items

71c1ff4

When no match is found and not filtering, closest is first item

79ac478

Prevent unecessary re-renders

fd53e86

Match USWDS behavior on enter with option selected

1782f8f

Match USWDS behavior of styling closest match if selected is not present

16ef4fe

Add test verifying exact match overwrite previous selection

8a5bbab

trussworks-infra-zz temporarily deployed to storybook April 15, 2021 03:09 Inactive

Update test name to be mrore clear

ba59881

trussworks-infra-zz temporarily deployed to storybook April 15, 2021 03:18 Inactive

brandonlenz commented Apr 15, 2021

View reviewed changes

brandonlenz marked this pull request as ready for review April 15, 2021 03:48

brandonlenz requested review from ahobson, gidjin, haworku, sarboc, SirenaBorracha, suzubara and tinyels as code owners April 15, 2021 03:48

trussworks-infra-zz temporarily deployed to storybook April 19, 2021 18:53 Inactive

Add test confirming filters are properly reset

684afb9

trussworks-infra-zz temporarily deployed to storybook April 19, 2021 19:13 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

60ff4af

trussworks-infra-zz temporarily deployed to storybook April 19, 2021 19:30 Inactive

brandonlenz added 2 commits April 19, 2021 15:30

Revert now-unecessary variable refactor

bd36ad3

Merge branch 'bl-time-picker-combo-box-filtering-1108' of github.com:…

02b7d66

…trussworks/react-uswds into bl-time-picker-combo-box-filtering-1108

trussworks-infra-zz temporarily deployed to storybook April 19, 2021 19:34 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

dabbbff

trussworks-infra-zz temporarily deployed to storybook April 20, 2021 20:00 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

1cb0d0a

trussworks-infra-zz temporarily deployed to storybook April 21, 2021 14:08 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

426e602

trussworks-infra-zz temporarily deployed to storybook April 26, 2021 14:58 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

574e832

trussworks-infra-zz temporarily deployed to storybook May 3, 2021 20:17 Inactive

Merge branch 'main' into bl-time-picker-combo-box-filtering-1108

9ffac9f

trussworks-infra-zz temporarily deployed to storybook May 6, 2021 17:58 Inactive

haworku previously approved these changes May 6, 2021

View reviewed changes

brandonlenz added 2 commits May 6, 2021 18:09

Complete test expectation

71e8d80

Check for clear button to be present after blur

7812a6d

brandonlenz dismissed haworku’s stale review via 7812a6d May 6, 2021 22:13

trussworks-infra-zz temporarily deployed to storybook May 6, 2021 22:17 Inactive

haworku approved these changes May 7, 2021

View reviewed changes

brandonlenz merged commit c684844 into main May 7, 2021

brandonlenz deleted the bl-time-picker-combo-box-filtering-1108 branch May 7, 2021 16:51

haworku mentioned this pull request Jun 14, 2021

chore: Merge branch 'V2' into main #1240

Merged

brandonlenz mentioned this pull request Jun 15, 2021

chore(release): 2.0.0 #1323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix TimePicker filter behavior. Misc ComboBox Fixes #1139

fix: Fix TimePicker filter behavior. Misc ComboBox Fixes #1139

brandonlenz commented Apr 15, 2021 •

edited

Loading

brandonlenz Apr 15, 2021 •

edited

Loading

brandonlenz commented Apr 15, 2021

brandonlenz commented Apr 19, 2021 •

edited

Loading

brandonlenz commented Apr 20, 2021

haworku left a comment •

edited

Loading

haworku May 6, 2021

haworku May 6, 2021

brandonlenz May 6, 2021

haworku May 6, 2021

brandonlenz May 6, 2021

haworku May 6, 2021

fix: Fix TimePicker filter behavior. Misc ComboBox Fixes #1139

fix: Fix TimePicker filter behavior. Misc ComboBox Fixes #1139

Conversation

brandonlenz commented Apr 15, 2021 • edited Loading

Summary

Related Issues or PRs

How To Test

brandonlenz Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

brandonlenz commented Apr 15, 2021

brandonlenz commented Apr 19, 2021 • edited Loading

brandonlenz commented Apr 20, 2021

haworku left a comment • edited Loading

Choose a reason for hiding this comment

haworku May 6, 2021

Choose a reason for hiding this comment

haworku May 6, 2021

Choose a reason for hiding this comment

brandonlenz May 6, 2021

Choose a reason for hiding this comment

haworku May 6, 2021

Choose a reason for hiding this comment

brandonlenz May 6, 2021

Choose a reason for hiding this comment

haworku May 6, 2021

Choose a reason for hiding this comment

brandonlenz commented Apr 15, 2021 •

edited

Loading

brandonlenz Apr 15, 2021 •

edited

Loading

brandonlenz commented Apr 19, 2021 •

edited

Loading

haworku left a comment •

edited

Loading