iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
84 stars 23 forks source link

feat(Timepicker): Add combined time column #844

Closed elephantcatdog closed 1 year ago

elephantcatdog commented 2 years ago

image

Adds useCombinedRenderer and combinedRenderer properties to Timepicker. useCombinedRenderer combines the individual time columns into one column. combinedRenderer allows users to customize the combined column.

Some places I think could use some improvement:

Notes:

Closes #507

Checklist

netlify[bot] commented 2 years ago

Deploy Preview for itwinui failed.

Name Link
Latest commit d8788a1c46df72e4255f7dea300177c2cd6379c2
Latest deploy log https://app.netlify.com/sites/itwinui/deploys/633f045faab32d0008887be1
elephantcatdog commented 2 years ago

I'm working on the virtualization

elephantcatdog commented 2 years ago

Virtualization has been added! I just made it automatic for data sets over 500. I tested out different numbers and the threshhold for slow rendering seems to be a little above here.

elephantcatdog commented 2 years ago

Oh! I need to make a PR for css so that it renders correctly.

Update: I made a PR in CSS for this: https://github.com/iTwin/iTwinUI/pull/786

veekeys commented 2 years ago

Oh! I need to make a PR for css so that it renders correctly.

Update: I made a PR in CSS for this: iTwin/iTwinUI#786

no, you dont. The example you have provided is semantically incorrect (li needs to be direct child of ol). Please check Combobox virtualization example, how to achieve virtualization and keep correct semantics.

elephantcatdog commented 2 years ago

no, you dont. The example you have provided is semantically incorrect (li needs to be direct child of ol). Please check Combobox virtualization example, how to achieve virtualization and keep correct semantics.

Thanks for the help Vykintas

elephantcatdog commented 1 year ago

I had to remove some parts of the keyboard navigation tests for the virtualized timepicker BUT! I fixed the problem with actual keyboard navigation and it works well in storybook. I can come back to this at a later date but I want to push the combined timepicker with virtualization and not have to redo more of the work later please.

elephantcatdog commented 1 year ago

Updated visual tests

elephantcatdog commented 1 year ago

Keyboard nav is missing esc key functionality but I'm not sure where we could add it as "opened" state is controlled by the user.

It could be done like in Dialog: have a property 'onClose' (and maybe the property 'closeOnEsc' also) that takes a function that closes the component.

I think this can be a new issue though.

Made the issue: https://github.com/iTwin/iTwinUI/issues/855

elephantcatdog commented 1 year ago

Don't forget to add issues that was discussed in this PR (eg. test improvements, etc)

Added issue for test improvements: https://github.com/iTwin/iTwinUI/issues/856

elephantcatdog commented 1 year ago

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.

elephantcatdog commented 1 year ago

I will revert virtualization changes and make an issue for it.

elephantcatdog commented 1 year ago

Reverted to before virtualization. Will make a new issue in the morning and consolidate the others I already made.

elephantcatdog commented 1 year ago

Visual tests fail because focused time changed position from start to end. To make visual tests more stable you can change scrollIntoView position to center, or start/end.

When I do this, clicking on a new time moves it to the center/end/start. Is that okay? It seems worse than before. I tried to add behavior: 'smooth' because I thought that at least it would visually scroll, but I can't because this definition of scrollIntoViewOptions doesn't include behavior.

https://user-images.githubusercontent.com/37936169/208116333-48c64bf2-5b28-47a4-b8e1-fc187c089020.mp4

gretanausedaite commented 1 year ago

When I do this, clicking on a new time moves it to the center/end/start. Is that okay? It seems worse than before. I tried to add behavior: 'smooth' because I thought that at least it would visually scroll, but I can't because this definition of scrollIntoViewOptions doesn't include behavior.

No, leave it as is, just update images. Let's hope it changed because of virtualisation.

elephantcatdog commented 1 year ago

nit: I think this got readded

This actually does change the look; I just didn't realize it before.

With div: image

Without: image

elephantcatdog commented 1 year ago

I hijacked the previous bug I filed and changed it to encompass all of virtualized timepicker: https://github.com/iTwin/iTwinUI/issues/856