Fixes #7021 - Refactoring Edit Events React code

[coder13]

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

• [x] Codesplit
• [ ] Fix eslint errors [ongoing]
• [x] Class components -> function components
• [x] Change round count
• [x] Remove event button
• [x] Add state management
• [x] Change scramble count
• [x] Change round format
• [x] EditCutoffModal
• [x] EditTimeLimitModal
• [x] EditAdvancementConditionModal
• [x] 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

[viroulep]

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]

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]

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]

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.

[gregorbg]

Picking up a discussion from Slack, as I believe we have a wider audience on GitHub:

The biggest blocker I'm running into right now with the Edit Events PR is with porting over the awesome keyboard navigation which appears that we got out of the box. I believe I'm possibly running into this issue regarding it and semantic ui https://github.com/Semantic-Org/Semantic-UI-React/issues/3745

After taking a brief look at the issue this seems like it ain't gonna be fixed "just like that". Caleb also voiced his concerns by saying

"Upgrading" to Semantic UI should not result in a loss of features and I'm unfortunately finding it difficult to maintain the slick keyboard navigation we've had on that page.

and I agree. @viroulep I believe you were involved in the decision to move from Bootstrap-React to Semantic, is that correct? Can you give us a bit of context why this decision was made and whether this issue with accessibility was something that you were aware of?

[viroulep]

I believe you were involved in the decision to move from Bootstrap-React to Semantic, is that correct? Can you give us a bit of context why this decision was made and whether this issue with accessibility was something that you were aware of?

To be accurate: the discussion was never to migrate from "something react" to "something else react", the discussions were about moving from "something" (bootstrap) to "something else". The consequence of this is that we based our decision on what the original framework offered feature-wise, and if it had a decent react adapter.

We were obviously not aware of that accessibility issue, and this is very unfortunate :( For more details about the decision process, can you check out the #sw-website-redesign on slack, around August 8th 2019? I unfortunately don't have time to do a "sum up" post right now, but there are a lot of discussions about that there!

I haven't looked at exactly how navigating with keyboard was made possible for our events editor, has someone looked into what tag attributes we would need to add and if it would be possible to add them to our component or even add them to react-semantic-ui?

[gregorbg]

Thanks a lot for your feedback! You don't have to provide a written summary of these arguments at all, simply pointing us in the right direction like you have done is all I had wished for :heart_eyes:

In terms of proceeding with this PR: The top priority is to get rid of Bootstrap-React and the rootRender hack. If this means renouncing keyboard controls for the time being, then I believe this is a price we have to pay. We can still look into adding the tags. but please consider that only as a "+1" and evaluate whether it would be too much work before actually diving into it.

[coder13]

In terms of proceeding with this PR: The top priority is to get rid of Bootstrap-React and the rootRender hack. If this means renouncing keyboard controls for the time being, then I believe this is a price we have to pay. We can still look into adding the tags. but please consider that only as a "+1" and evaluate whether it would be too much work before actually diving into it.

I'll give it a chance. I've been doing some keyboard navigation features for one of my side projects and can look into the difficulty of micromanaging input focuses by listening to the keyboard. If you don't think this is a good idea, I won't proceed and will rather tidying the PR up and make it ready for review.

[gregorbg]

"micromanaging input focuses by listening to the keyboard" sounds like it's way too much work. Quick googling reveals the tabindex property, how is that different from what you're trying to achieve?

Playing around with assigning some properties like tabindex is fine. Writing full-blown keyboard listeners is definitely too much (and at the risk of overstretching my frontend experience, also doesn't seem like it's the easiest solution to the problem)

[gregorbg]

@coder13 Where are we standing on this? Regulations changes coming up that rely on changing the Events UI. It would be fantastic if we can have this merged soon :blush:

[coder13]

@coder13 Where are we standing on this? Regulations changes coming up that rely on changing the Events UI. It would be fantastic if we can have this merged soon blush

I don't know that I have time to work on this unfortunately. I overestimated the amount of work I can take on. This is about 95% of the way there though and anyone who was going to touch this code for the regulation changes would likely be able to get it all the way to the finish line.

[kr-matthews]

The result input is now working 'properly' since it has the event id passed to it (before it was always defaulting to a time field input, which is incorrect for FM and MBF). At this point, specifying qualifications technically works. However, the input for MBF is still the standard component for accepting a full result: solved, total, and time. There's a note saying to enter n / n in any time, and that the time will be ignored. Also, the UI for this part looks not as good as the rest.

I can replace this to just accept a single input, to specify the points, and then convert that number to a MBF result behind the scenes. Which is what we ultimate want. But that would require creating new components and more special cases, which would take a fair bit more time. It might be easier to have that as a separate PR, once https://github.com/thewca/wcif/issues/9 has been resolved, and get this merged sooner rather than later?

In either case, it would be good if a few other people started testing it out - create a new competition and add a bunch of events with varying cutoffs, qualifications, etc., and modify existing competitions.

I also fixed a few bugs and some UI.

[gregorbg]

Only one bug that I've found so far: When adding Fewest Moves to an event, Mo3 is selected by default as format. But choosing another format in the dropdown changes nothing. It always stays Mo3, no matter what I click. For other events, format selection works as expected.

[kr-matthews]

Only one bug that I've found so far: When adding Fewest Moves to an event, Mo3 is selected by default as format. But choosing another format in the dropdown changes nothing. It always stays Mo3, no matter what I click. For other events, format selection works as expected.

Format selection was broken for all events, though it's only noticeable in FM and MBF I think. Should be fixed now.

Not sure why these two tests are failing now, my changes were simple and unrelated as far as I can tell.

[kr-matthews]

Based on email/slack discussion, I've updated the qualification input for multi to only accept an integer specifying the number of points, and that gets converted to n/n in 99999 time (unknown time). Note that the cut-off for multi asks you to input a full multi result - should that also be changed to the simplified input used in qualification?

FM qualification input for average was a bit off because it wasn't taking into account that averages are stored off by a factor of 100. I've fixed that.

There's also one bug where if you type in 198 as the multi points qualification then on blue the input jumps to 396. And 999 jumps to 198. This seems to happen a lot, but not for any 'reasonable' inputs you'd use in practice. But I will investigate and fix this next.

[gregorbg]

Note that the cut-off for multi asks you to input a full multi result - should that also be changed to the simplified input used in qualification?

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).

There's also one bug where if you type in 198 as the multi points qualification then on blue the input jumps to 396. And 999 jumps to 198. This seems to happen a lot, but not for any 'reasonable' inputs you'd use in practice. But I will investigate and fix this next.

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.

[coder13]

I have no new thoughts on the react code. It LGTM.

[kr-matthews]

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]

@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]

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]

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.