Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtualized TimePicker #856

Open
elephantcatdog opened this issue Dec 15, 2022 · 1 comment
Open

Virtualized TimePicker #856

elephantcatdog opened this issue Dec 15, 2022 · 1 comment
Labels
enhancement New feature or request react Needs change in react package

Comments

@elephantcatdog
Copy link
Contributor

elephantcatdog commented Dec 15, 2022

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: iTwin/iTwinUI-react#844

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

Notes about the problems I was facing:

  • The lines expect(document.activeElement).toEqual(selectedTime); in TimePicker.test.tsx do not work with the virtualization. I made an issue previously about this, but removed it and consolidated that into this issue. The text of that issue is a comment on this issue.
  • Also mentioned in the previous issue - it would be nice if the tests for the virtualized timepicker were integration tests instead of mocked tests. I tried to do this initially but couldn't get it to work.
  • The problem that stopped work on virtualization was scrolling. Mouse, trackpad, and keyboard arrow keys - none allowed scrolling past the selected item unless you scrolled really hard and continuously; eventually you can get unstuck, but... that's not a solution. There is more info about this in the the PR.
  • Another problem I found is that if the selected time is not 'visible' in the DOM, pressing tab from the menu will not allow you to enter the time dropdown. (ie scroll so that selected time is not visible, then trying to tab to it does not work because apparently unselected times are not allowed to be tabbed to)
@elephantcatdog elephantcatdog added the bug Something isn't working label Dec 15, 2022
@elephantcatdog elephantcatdog changed the title Improve testing for combined/virtualized TimePicker Virtualized TimePicker Dec 16, 2022
@elephantcatdog elephantcatdog added enhancement New feature or request and removed bug Something isn't working labels Dec 16, 2022
@elephantcatdog
Copy link
Contributor Author

elephantcatdog commented Dec 16, 2022

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.

@gretanausedaite gretanausedaite added the react Needs change in react package label Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request react Needs change in react package
Projects
None yet
Development

No branches or pull requests

2 participants