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

Migrate Rankings Table to React #10569

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

FinnIckler
Copy link
Member

We completely forgot about this one!
image
This already works fine if we just want to get rid of the bootstrap table. But I think we should also just invest the time and fix up the filters to get the speed boost from not rerendering the page

@FinnIckler
Copy link
Member Author

image
I added the functionality of filters and loading the data from the frontend.
Styling for the filters are still TODO though

@FinnIckler
Copy link
Member Author

I think removing the segment makes sense
image

@FinnIckler
Copy link
Member Author

Screenshot 2025-01-10 at 14 58 47 Styled the filters based on the competitions page

@FinnIckler
Copy link
Member Author

Screenshot 2025-01-10 at 16 33 35 Here is the by region view

@FinnIckler
Copy link
Member Author

I think that means I am done?

Copy link
Contributor

@kr-matthews kr-matthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes about a minute to load the data when I change any filters. Is that just a running-locally problem?

app/webpacker/components/wca/EventSelector.jsx Outdated Show resolved Hide resolved
app/webpacker/lib/requests/routes.js.erb Outdated Show resolved Hide resolved
app/webpacker/components/Results/Rankings/index.jsx Outdated Show resolved Hide resolved
app/webpacker/components/Results/Rankings/index.jsx Outdated Show resolved Hide resolved
app/webpacker/components/Results/resultsFilter.jsx Outdated Show resolved Hide resolved
hideAllButton
hideClearButton
/>
<RegionSelector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component should be moved to components/wca (maybe in separate PR; the dispatchFilter prop should probably be changed to onChange, since you don't need a reducer/dispatcher to use this reusable component).

@FinnIckler
Copy link
Member Author

It takes about a minute to load the data when I change any filters. Is that just a running-locally problem?

Yes and no. It is quite slow to compute rankings always, which is why we precompute the pages in prod after every cad, but current local state makes it about 5 * worse because of the Debugging output and it also currently serializes the competition object very expensively

@dunkOnIT
Copy link
Contributor

@thewca-bot deploy staging

@FinnIckler
Copy link
Member Author

@thewca-bot deploy staging

@FinnIckler
Copy link
Member Author

It takes about a minute to load the data when I change any filters. Is that just a running-locally problem?

Yes and no. It is quite slow to compute rankings always, which is why we precompute the pages in prod after every cad, but current local state makes it about 5 * worse because of the Debugging output and it also currently serializes the competition object very expensively

Just improved the speed by 10x

@dunkOnIT
Copy link
Contributor

dunkOnIT commented Jan 13, 2025

10x speedup sounds amazing!

Two mobile issues:

  • Table should be marked unstackable
  • Filter buttons overflow their boundary in mobile view (see pic below)

image

  • Issue with region display, see below picture
    image

@FinnIckler
Copy link
Member Author

fixed the mobile issues

@dunkOnIT
Copy link
Contributor

dunkOnIT commented Jan 14, 2025

Some more issues:

  • (fig 1) Have to scroll horizontally to see the results on mobile - we should be able to see at least the name and the time achieved. One option is just making the table wider (see Fig 2 for reference to old table)
  • Multi BLD average button shows the round info from when the person achieved the MBLD single oop

Fig 1:
image

Fig 2:
image

Copy link
Contributor

@kr-matthews kr-matthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused why there was a duplicate key before - doesn't that mean that a result was showing up in the table twice? I don't understand how switching to react table fixed it either.

@FinnIckler
Copy link
Member Author

I'm still confused why there was a duplicate key before - doesn't that mean that a result was showing up in the table twice? I don't understand how switching to react table fixed it either.

Well I assumed that there was a edge case where the current key wasn't unique enough and react table takes care of that

@FinnIckler
Copy link
Member Author

Also I wasn't just switching to React Table to fix that. I was talking to Gregor and the goal is to use it everywhere

Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also merge main so that you can get the linter to rumble about your JSX files 😉

@kr-matthews
Copy link
Contributor

I'm still confused why there was a duplicate key before - doesn't that mean that a result was showing up in the table twice? I don't understand how switching to react table fixed it either.

Well I assumed that there was a edge case where the current key wasn't unique enough and react table takes care of that

My point is that a duplicate key would mean a result was showing up twice, since the key was using the id, which seems wrong - how could such an edge case exist? The switch to react table doesn't change that fact.

@FinnIckler
Copy link
Member Author

I'm still confused why there was a duplicate key before - doesn't that mean that a result was showing up in the table twice? I don't understand how switching to react table fixed it either.

Well I assumed that there was a edge case where the current key wasn't unique enough and react table takes care of that

My point is that a duplicate key would mean a result was showing up twice, since the key was using the id, which seems wrong - how could such an edge case exist? The switch to react table doesn't change that fact.

Did you actually see any duplicate results? I assume this is a problem with the query when one average contains two singles that are shown in the results view. I can dig deeper tomorrow

@kr-matthews
Copy link
Contributor

Did you actually see any duplicate results? I assume this is a problem with the query when one average contains two singles that are shown in the results view. I can dig deeper tomorrow

No, but I didn't look very closely/carefully. This was for displaying 3x3x3 single. If the id of a result is coming from the average, then that could explain it. But in that case, it indicates that the key needs to be updated. If moving to react table has resolved the issue, despite a duplicate key existing, then surely it's not using the key and we can remove the key (I see it's still in the code)? (And if it is using the key, then how is it not getting duplicates?)

Basically, it might (currently) work, but there's definitely a (potentially minor) underlying issue somewhere, because the key isn't unique.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants