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

Match WIP (QUI-11) #98

Merged
merged 51 commits into from Jun 23, 2023
Merged

Match WIP (QUI-11) #98

merged 51 commits into from Jun 23, 2023

Conversation

boehs
Copy link
Contributor

@boehs boehs commented Jun 21, 2023

I haven't worked on this for a bit so I'm just pring what I have. I will get back around to it. ok so apparently this is how you end procrastination

  • Match controller
  • Match UI
  • Sync view to controller
  • Validate drops
  • Scatter on invalid
    • Fade on correct
  • End screen
  • Progress sidebar
  • Leaderboard Component
    • Backend
      • Sync to backend
      • Hydrate lb

Also, there's a hack where

  1. Card sizes are hardcoded
  2. Window size is hardcoded

Here's a little vid of what I have

https://github.com/miapolis/quizlet.cc/assets/51836263/6d931d7c-ca2b-4891-a99f-e746938c6a4b

Screen.Recording.2023-06-21.at.7.22.53.PM.mov

Some implementation details:

  1. There's a currently unused framework to mark rounds as ineligible for leaderboards. This is so settings can be introduced like study starred, ect

@vercel
Copy link

vercel bot commented Jun 21, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @miapolis on Vercel.

@miapolis first needs to authorize it.

@boehs
Copy link
Contributor Author

boehs commented Jun 21, 2023

I think the leaderboard can be skipped for now and the extra settings as well, but the other stuff still needs to be done

@boehs
Copy link
Contributor Author

boehs commented Jun 21, 2023

Oh also it performs fine I think but also I have no clue what I'm doing react is terrible so like

@miapolis
Copy link
Member

Overall, looks like a great start just make sure you're running the formatter. Good idea to mark rounds as ineligible for leaderboards if it is customized somehow, I didn't think of that.

Also, just looking at the video, if you haven't done this already make sure that the bottom border changes color as well when the term is completed or incorrect.

@miapolis miapolis marked this pull request as draft June 21, 2023 17:10
@boehs
Copy link
Contributor Author

boehs commented Jun 21, 2023

Also, just looking at the video, if you haven't done this already make sure that the bottom border changes color as well when the term is completed or incorrect.

Just did in a terrible way

@uncenter
Copy link

Also, just looking at the video, if you haven't done this already make sure that the bottom border changes color as well when the term is completed or incorrect.

Just did in a terrible way

oh...

@boehs boehs changed the base branch from main to staging June 21, 2023 19:28
@miapolis
Copy link
Member

@boehs I refactored a ton and the build is now passing with linter enabled. There's still the bug with random pairs not reacting every once in a while, look more into that.

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Should the timer wait 0.5 seconds before starting? (the animation where cards move in)

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Actually here's an interesting one: I was curious if you could match while cards are moving. Turns out, you can grab cards in motion, but if you successfully do so the bounding box calculations are broken for that card thereafter.

I propose

  1. Disabling pointerevents for 0.5 seconds on cards in motion (hacky fix)
  2. Delaying timer start for 0.5 seconds

Alternatively we could hunt down this bug and make it so nerds with either

  1. A 1000hZ monitor
  2. Like a 1992 IBM thinkpad or smth

can match cards while still in motion

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

I ended up implementing the proposed solution in d018727. Another thing: currently the presented score is a little kind. A time of 9.998 will be displayed as 9.9 instead of 10.0. This number could be rounded, or it could continue to be kind.

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

I ended up implementing the proposed solution in

feels a little off tbh. In the future we can mess with reducing the offsets to be somewhere between. It's a one line change.

image

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Unless there's more critique, this is ready, minus leaderboards which are mostly done but not linked up yet, but merge without leaderboards.

Also

Another thing: currently the presented score is a little kind. A time of 9.998 will be displayed as 9.9 instead of 10.0. This number could be rounded, or it could continue to be kind.

feels a little off tbh. In the future we can mess with reducing the offsets to be somewhere between. It's a one line change.

but those can both be fixed easily later. All this feels ready for me. Send your final reviews

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Oops, I realized in small enough browser windows it will never load because there's just not enough free space to find empty. I'm going to add a timeout of 100 attempts per card added timeout. also avoiding spawning ontop of timer now

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Screen.Recording.2023-06-23.at.2.16.54.AM.mov

Updated video

@miapolis
Copy link
Member

Much better! A few things however: the very first time Match is opened, there needs to be a modal with a start button so that the timer doesn't start immediately. There's also another bug where the New Match button restarts the round but new cards don't spawn in. Fix these two things and I think we're ready to merge into staging, and after leaderboard and backend work is done I'll push to prod.

image

@miapolis
Copy link
Member

miapolis commented Jun 23, 2023

@boehs I ran prettier and moved GridStat into its own file. I read your comment about using refs to keep track of delayed state and it's not uncommon at all in React—I think I do that quite a lot in this codebase already

@miapolis
Copy link
Member

I also just realized, I know what looks off: every card is the same width, even if it's only a few characters. It'll be a little more involved to handle varying widths for cards but I think you'll be fine, the current fixed width should instead be the max width. For staging here are the last three things:

  • Start modal when Match is clicked from the main set page (so timer doesn't start immediately) - on reload it's fine if it starts right away
  • Fix 'New Match' button not spawning in new cards
  • Cards take up the min width they need to occupy, current width becomes the max width instead

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Fix 'New Match' button not spawning in new cards

Ugh when did this happen

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

I can't check your boxes so here they are again!

  • Start modal when Match is clicked from the main set page (so timer doesn't start immediately) - on reload it's fine if it starts right away
  • Fix 'New Match' button not spawning in new cards
  • Cards take up the min width they need to occupy, current width becomes the max width instead

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

Ok ready for review part 3!

Eventually the match dialog should show a short video explaining match, like this

Screen.Recording.2023-06-23.at.11.41.14.AM.mov

and <Enter> should start a new game, but it's good for now imo

@miapolis
Copy link
Member

@boehs I fixed a bunch of things, window resizing handling now works as well. I've noticed a few bugs however relating to z indices once again, especially with sets with fewer than 6 terms... a lot of cards don't seem to have the highest order z-index when being dragged in these scenarios

@boehs
Copy link
Contributor Author

boehs commented Jun 23, 2023

LGTM, resize handling is great. Debugging the z-index stuff is difficult and I'm struggling to reproduce. I'm tempted to chalk it up to HMR

@miapolis miapolis merged commit f27b574 into quenti-io:staging Jun 23, 2023
1 check failed
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.

None yet

3 participants