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

refactor: cuesheet v2 #435

Merged
merged 72 commits into from
Jul 22, 2023
Merged

refactor: cuesheet v2 #435

merged 72 commits into from
Jul 22, 2023

Conversation

cpvalente
Copy link
Owner

@cpvalente cpvalente commented Jun 15, 2023

This PR aims to redesign the cuesheet to the new system
It should also add row virtualisation to improve performance on handling several hundreds of items

TODO:

  • write cache tests
  • unify delay values into columns

@cpvalente
Copy link
Owner Author

cpvalente commented Jun 20, 2023

Hi @asharonbaltazar , hoping to ask for some help here:

I am working on rebuilding the current OntimeTable at /cuesheet
This, admittedly slightly too large PR, includes several changes migrating to react-table v8 as well as style changes to follow the rest of the app. The only relevant piece of code that I would like your review is in the <Cuesheet /> component

I am slightly stunned on finding a good solution for optimising the table here, in the current PR I have attempted to use react-virtual (also tanstack) since it seemed efficient and lightweight
However the UX is less than ideal with several the user experiences jerks in animation
I would also need to write custom logic to measure the rows individually since all rows have variable height 🤦

In the rundown view we use IntersectionObserver in the EventBlock to prevent rendering the expensive inner components if not in view, resulting in rendering just the outer div
This works well performance wise and simple to maintain
I wonder if this would be a bad approach as far as tables go though

In terms of specification, the expectation here is that users would have slightly under 1000 elements in the table and will want the fastest experience in re-renders possible. Initial render time is less problematic

Any thoughts?

the ux with react-virtual was not great, the library is due a major release so we will try again later
@asharonbaltazar
Copy link
Collaborator

Ack. Taking a look later today 👀

@cpvalente
Copy link
Owner Author

Sorry, should've been more specific! I was referring to the settings, where the first items under TOGGLE FIELD VISIBILITY and TABLE OPTIONS both have left padding. I think you should remove the left padding because it looks out of place

hahaha, no need to apologise.
I understood and agree.

The question above was unrelated to the padding changes, I should have been the one to be more specific :D

@asharonbaltazar
Copy link
Collaborator

But I am unsure that the current solution provides a good enough indication

Nah, it looks cleaner than the old one.

I'm wondering if the old one would look better if you fade the subsequent rows' color a bit... Sorta like the contrast in these buttons

@cpvalente cpvalente marked this pull request as ready for review July 21, 2023 21:47
@cpvalente cpvalente merged commit 3603b83 into master Jul 22, 2023
2 checks passed
@cpvalente cpvalente deleted the refactor/cuesheet-v2 branch July 22, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants