iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
105 stars 38 forks source link

Virtualized TimePicker #856

Open elephantcatdog opened 1 year ago

elephantcatdog commented 1 year ago

The combined timepicker needs to have virtualization added as it's very slow to open and respond because of the amount of data/times in the dropdown.

I started this, and the work I did is copied onto the branch leah/customize-time-picker-copy-with-virtualization. [Update: This branch might be on the iTwinUI-react project instead of this one, iTwinUI.] There were quite a few problems I encountered when doing the virtualization. Most of them should be documented on the combined timepicker column PR: https://github.com/iTwin/iTwinUI-react/pull/844

A related issue I made is Escape keyboard functionality in TimePicker and DatePicker

Notes about the problems I was facing:

elephantcatdog commented 1 year ago

Previous issue description:

Steps to reproduce

  1. Open a local version of iTwinUI (react)
  2. Open 'TimePicker.test.tsx'
  3. Uncomment the commented out lines // expect(document.activeElement).toEqual(selectedTime); found in two tests 'should navigate with keyboard in combined renderer' and 'should navigate with keyboard in combined renderer (12 hours)'
  4. Run tests

Actual behavior

The activeElement in the document is the body.

Expected behavior

The activeElement in the document should be the selected time.

Additional information

From Combined TimePicker PR

The related tests for the regular version of the timepicker ('should navigate with keyboard' and 'should navigate with keyboard (12 hours)') work for this test. This part of the test failing doesn't seem to indicate any actual problems with the component, as far as I can tell, though. Maybe it's not important? [UPDATE: I now think this might be related to the scrolling problem]

Also, it'd be nice if the tests for the combined/virtualized timepicker were integrated tests rather than mocked tests. I tried to do this initially and it didn't work out.