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

Fixes #7021 - Refactoring Edit Events React code #7025

Merged
merged 82 commits into from
Sep 19, 2023

Conversation

coder13
Copy link
Contributor

@coder13 coder13 commented Jun 30, 2022

Wanted to create this draft PR to get started.

My first mission was to start cleaning up the code.
My next will be to start migrating these class components to functional components. This will, for most of them, completely redo their logic and how they pass around data, but it does it in a strategic way where react knows what has updated and will make the rootRender call redundant. I also want to make sure we're maximizing code-reuse to potential reuse some of these components in other parts of the website where relevant.

Checklist

  • Codesplit
  • Fix eslint errors [ongoing]
  • Class components -> function components
  • Change round count
  • Remove event button
  • Add state management
  • Change scramble count
  • Change round format
  • EditCutoffModal
  • EditTimeLimitModal
  • EditAdvancementConditionModal
  • EditQualificiationModal
  • Figure out autofocus again as well as keyboard navigation
  • Bug fix
  • Fix MBLD input to only ask for points for cutoff and qualification
  • Address future merge conflicts

@coder13 coder13 changed the title [#7021] Refactoring: fixing eslint problems, code splitting Fixes #7021: Refactoring: fixing eslint problems, code splitting Jun 30, 2022
@coder13 coder13 changed the title Fixes #7021: Refactoring: fixing eslint problems, code splitting Fixes #7021 - Refactoring: fixing eslint problems, code splitting Jun 30, 2022
@coder13 coder13 changed the title Fixes #7021 - Refactoring: fixing eslint problems, code splitting Fixes #7021 - Refactoring Edit Events React code Jun 30, 2022
@coder13 coder13 linked an issue Jun 30, 2022 that may be closed by this pull request
@dunkOnIT dunkOnIT added this to the Focuses for July milestone Jul 1, 2022
@viroulep
Copy link
Contributor

viroulep commented Jul 5, 2022

Hey @coder13, I was just passing by to see how painful it was to migrate that old code, and I noticed you had to create some "results" input (for times, points, etc...).
You may want to re-use AttemptResultField which already has some logic to help the user input some result and convert this input to the actual value we expect.
You can check out how it's used here (and you can check out the single result edition page to see how it renders).

I don't know if it will integrate well in the "edit event" modals and stuff, but I'd figure it's a quick thing to try to re-use some components.

@coder13
Copy link
Contributor Author

coder13 commented Jul 5, 2022

Hey @coder13, I was just passing by to see how painful it was to migrate that old code, and I noticed you had to create some "results" input (for times, points, etc...). You may want to re-use AttemptResultField which already has some logic to help the user input some result and convert this input to the actual value we expect. You can check out how it's used here (and you can check out the single result edition page to see how it renders).

I don't know if it will integrate well in the "edit event" modals and stuff, but I'd figure it's a quick thing to try to re-use some components.

I was thinking this might be good for a follow-up PR. @gregorbg said to copy it exactly as it was so that was my main focus but I agree to reuse code and especcially this attemptResultField input. It might integrate well.

@gregorbg
Copy link
Member

gregorbg commented Jul 6, 2022

My intention behind the "copy as it is" instruction was to limit the scope of this PR, so that you can get a clear sense of what you have to do. I didn't want to give you the feeling that you'd have to reinvent the wheel as your first task.
With that in mind, if there's a place where you feel you can reuse some newer code that was introduced after this feature was originally created, then go for it! As long as it's a "low hanging fruit" that doesn't generate a high amount of extra work I'm fine :)

@coder13
Copy link
Contributor Author

coder13 commented Jul 7, 2022

My intention behind the "copy as it is" instruction was to limit the scope of this PR, so that you can get a clear sense of what you have to do. I didn't want to give you the feeling that you'd have to reinvent the wheel as your first task. With that in mind, if there's a place where you feel you can reuse some newer code that was introduced after this feature was originally created, then go for it! As long as it's a "low hanging fruit" that doesn't generate a high amount of extra work I'm fine :)

Gotcha, I'll keep this in mind.

@kr-matthews
Copy link
Contributor

I have never seen a cutoff for MBLD used in practice, but I'd argue that whatever counts for qualification results also counts for cutoffs. In a sense, you are "qualifying" for the remaining attempt(s).

I've updated the cutoff and advancement conditions to both accept just points for multi, just like the qualification result already did.

The MBLD database format is not designed to accept values > 99 points. We only reserve two digits so values geq don't make sense. You can just limit the spinner at 99.

The largest value you can type in now is 99.

I think this is ready for a full review now. There will probably be some failing tests which I'll need help with since I still seem to get different results when testing locally.

@kr-matthews kr-matthews marked this pull request as ready for review May 26, 2023 16:10
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.

Magnificent refactor, and a great culmination of the joint effort between you and Cailyn! Frankly, I didn't have the opportunity to look through every single JS file. Overall, one theme I noticed were inconsistencies with

  • inline-CSS vs. separate CSS files via selectors
  • naming conventions: .round-row__scramble-set-count uses double-underscore and snake-case (or rather, dash-case) whereas .round-row_advancementCondition in the same file uses one underscore and camelCase

I didn't bother marking every single one of those occurences, but is there a reasoning behind this choice?

Other than that, I am generally fine with this PR once we test it on staging and automated CI tests pass.

.bundle/config Outdated Show resolved Hide resolved
WcaOnRails/.eslintrc.json Outdated Show resolved Hide resolved
WcaOnRails/app/models/user.rb Outdated Show resolved Hide resolved
WcaOnRails/app/views/competitions/edit_events.html.erb Outdated Show resolved Hide resolved
WcaOnRails/config/i18n-tasks.yml.erb Outdated Show resolved Hide resolved
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.

Magnificent refactor, and a great culmination of the joint effort between you and Cailyn! Frankly, I didn't have the opportunity to look through every single JS file. Overall, one theme I noticed were inconsistencies with

  • inline-CSS vs. separate CSS files via selectors
  • naming conventions: .round-row__scramble-set-count uses double-underscore and snake-case (or rather, dash-case) whereas .round-row_advancementCondition in the same file uses one underscore and camelCase

I didn't bother marking every single one of those occurences, but is there a reasoning behind this choice?

Other than that, I am generally fine with this PR once we test it on staging and automated CI tests pass.

@kr-matthews
Copy link
Contributor

@gregorbg I've addressed a couple of your comments, but most of them are parts that @coder13 wrote and I didn't touch at all, and I don't know the answer to. If those questions get answered/decided then I can make the necessary changes.

As I've mentioned, I'll probably need some help with the failing tests.

When would be a good time to test on staging? (Presumably after the above comments are sorted out.)

@kr-matthews
Copy link
Contributor

I believe I've addressed all the specific comments.

Unfortunately there's a bug with shared cumulative time limits. The same bug that we had on the previous (well, current) edit events page, which I fixed over a year ago :( So I'll have to deal with that.

@kr-matthews
Copy link
Contributor

kr-matthews commented Jul 8, 2023

Unfortunately there's a bug with shared cumulative time limits. The same bug that we had on the previous (well, current) edit events page, which I fixed over a year ago :( So I'll have to deal with that.

Fixed.
Edit: wow all the tests passed for once.

Copy link
Contributor

@dunkOnIT dunkOnIT left a comment

Choose a reason for hiding this comment

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

LGTM - have looked through test cases and QA'd on staging. Will merge and deploy on Monday (no deployments on Friday :P )

@dunkOnIT dunkOnIT merged commit f5ea525 into thewca:master Sep 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Event icons in "manage events" overlapping with event names editEvents component refactor
6 participants