Closed mhgamboa closed 2 years ago
mhgamboa
commented
2 years ago 
dumbmatter
commented
2 years ago Very cool! Nice idea to have it auto scroll with useEffect.
Some thing that I noticed playing around with it:
The animation when adding a new game is pretty choppy in this PR. Not sure why. Looks like it's confused and jumping around, rather than just smoothly adding in one game.
At the start of a new season the first game is left aligned rather than right aligned. It only looks right after enough games are played to fill the bar.
Or, if it's possible to make it scroll with the mouse wheel, could we just hide the scrollbar? I'm not sure if that's possible either. Currently the mouse wheel just scrolls the main page.
Also in the above screenshot, the padding on the right is too close against the button. It looks better like how it does without this PR:
Deleted manipulation of games2 to allow all previous games to be accessible
I wonder if that has any performance implications?
Overall, at least some of those would have to be fixed for me to merge this PR. Ideally all of them, but some I'm not sure are possible!
mhgamboa
commented
2 years ago Really appreciate the feedback. Let me see if I can tinker some more to get all of that for you
mhgamboa
commented
2 years ago Ok, I believe I have addressed your major concerns. Do I need to create another pull request, or is there a way to add onto this one? The branch "make-scoreboxess-scrollable" seems to have disappeared from my local environment. (It's my first time collaborating on github so bear with me)
dumbmatter
commented
2 years ago Ok, I believe I have addressed your major concerns. Do I need to create another pull request, or is there a way to add onto this one? The branch "make-scoreboxess-scrollable" seems to have disappeared from my local environment. (It's my first time collaborating on github so bear with me)
Weird that it disappeared, I don't know why that happened. But generally you keep the same PR open. Commits to the branch update the PR, as you can see here.
I haven't had a chance to look at this yet, but will try to within the next couple days!
dumbmatter
commented
2 years ago Ooh, this is getting nice!
What do you think about behavior when a new game is played... I think ideally it should be that if you're already scrolled all the way to the right, keep it scrolled all the way to the right. But if you're viewing something in the past, it should not scroll at all. Currently it gets the first part right (stays to the right when already there) but not the second part - if you're in the past when a game is simmed, it shifts what you're viewing by some amount, which seems not ideal.
Scroll wheel works, but it goes in the opposite direction as to what I'd expect. I'd expect "up" to be past games and "down" to be future games. Is it possible to switch that?
And a little bug - when the score boxes are closed, the title and dropdowns are now hidden too, which should not be the case:
Also what about performance? I haven't benchmarked it but I can. Actually that goes for everything... if you want to say "I'm done, you finish it" at any point, let me know! Or if you want to get it all perfect yourself, that's fine too :)
Same goes for the little TypeScript errors in your code - run yarn run lint to see them.
mhgamboa
commented
2 years ago Sounds good, I will probably look over all this over the coming days and get back to you.
Actually that goes for everything... if you want to say "I'm done, you finish it" at any point, let me know! Or if you want to get it all perfect yourself, that's fine too :)
I'll at least tackle some of what you've mentioned. This is actually really good practice for me as I've never had to benchmark for performance or manage typescript errors so I'd at least like to take a stab at it. I'll keep you informed as I continue to work
dumbmatter
commented
2 years ago I'll at least tackle some of what you've mentioned. This is actually really good practice for me as I've never had to benchmark for performance or manage typescript errors so I'd at least like to take a stab at it. I'll keep you informed as I continue to work
Awesome, take all the time you need and feel free to ask questions!
mhgamboa
commented
2 years ago And a little bug - when the score boxes are closed, the title and dropdowns are now hidden too, which should not be the case
I'm having trouble reproducing this, how do you create this bug? Does it happen everytime you click the toggle button to hide the scoreboxes? Or only when there is an overflow?
Awesome, take all the time you need and feel free to ask questions!
Junior developer question: What do you use to test performance? What metrics are you looking for? I know at this point it may just be easier for you to do it yourself than to explain the basics to me, but if you could point me in the right direction that would be a huge help for me as a developer 🙏
dumbmatter
commented
2 years ago What do you use to test performance?
In this case, I'd switch to a page where not much is going on (like Tools > Frivolities) and manually time (like with a stopwatch app) how long it takes to sim a season with the score box bar open. Actually might need more like 10 seasons due to randomness, not sure exactly. But basically I'd do that test before and after this PR. Ideally there is no change in performance. A small decrease in performance is probably acceptable. Large, probably not.
I'm having trouble reproducing this, how do you create this bug?
I was seeing it every time. I'll look at it again in a bit and try to figure out why.
dumbmatter
commented
2 years ago I'm having trouble reproducing this, how do you create this bug? Does it happen everytime you click the toggle button to hide the scoreboxes? Or only when there is an overflow?
Happens any time I close the bar, like in the screenshot. In Firefox and Chrome. Seems to be because the league-top-bar div goes to height 0 when closed now, when previously it didn't.
mhgamboa
commented
2 years ago Actually that goes for everything... if you want to say "I'm done, you finish it" at any point, let me know! Or if you want to get it all perfect yourself, that's fine too :)
@dumbmatter On 2nd thought I think I might just pass the rest off to you. I've had a couple things come up, and haven't had as much time to dedicate as thought I had. Plus there's that bug that I can't recreate on my end.
I hope my contribution was at least somewhat helpful, and I'm happy to answer any questions about my code in the future
dumbmatter
commented
2 years ago Thanks! I'm working on it. Btw I figured out the bug you couldn't reproduce (although I'm not sure why you couldn't reproduce it) - it's because you made the toggle thing absolutely positioned, so it no longer gave the parent div height when it was the only thing inside it.
dumbmatter
commented
2 years ago I fixed most of the stuff in https://github.com/zengm-games/zengm/tree/mhgamboa-make-scoreboxes-scrollable
Then benchmarking... interesting stuff!
The metric is how long it takes to sim an entire regular season while viewing the main Frivolities page.
master branch: 23 seconds mhgamboa-make-scoreboxes-scrollable branch: 28 seconds
So not good, that's a 20% increase! But why? Seems all of it is due to the animation effect when a game is simmed. Because if I get rid of the animation, then the benchmark is:
master branch: 19 seconds mhgamboa-make-scoreboxes-scrollable branch: 19 seconds either branch, even with the bar closed: 19 seconds
So making it scrollable is free, unless it's also animated, in which case making it scrollable costs about as much as making it animated - about 20%.
20% seems kind of large. Maybe the animation is not worth it in the first place? I'm leaning towards that, although I guess I'll sleep on it since it's late :)
mhgamboa
commented
2 years ago Wow! Very interesting. I'm excited to see how it all turns out 🙌