Make scoreboxes scrollable (#301) #400

mhgamboa · 2021-12-07T17:15:10Z

Edited toggle bar to overflow-auto (from overflow-hidden) to enable scrolling
Edited scss to ensure toggle button always remains on the right side of the page
Deleted manipulation of games2 to allow all previous games to be accessible
Added useEffect to always have scrollboxes scroll to the most current game (probabably more efficient way to scroll, as the useEffect triggers after every render)

dumbmatter · 2021-12-07T22:55:27Z

src/ui/components/LeagueTopBar.tsx

+
+	useEffect(() => {
+		if (show) {
+			const leagueTopBar = document.querySelector(".league-top-bar");


Ideally you would use a ref from React rather than document.querySelector.

dumbmatter · 2021-12-07T23:01:56Z

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.

The scrollbar looks too distracting, especially in dark mode. Maybe it could be made smaller with a custom scroll bar that fits the theme? I'm not sure how easy that is, I've never done it before.

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 · 2021-12-07T23:39:00Z

Really appreciate the feedback. Let me see if I can tinker some more to get all of that for you

mhgamboa · 2021-12-10T16:39:01Z

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)

mhgamboa · 2021-12-10T19:39:29Z

public/css/sidebar.scss

+		&::-webkit-scrollbar {
+			height: 3px;
+		}
+		&::-webkit-scrollbar-track {
+			background: #dadada;
+			border-radius: 50px;
+		}
+		&::-webkit-scrollbar-thumb {
+			background: rgba(71, 71, 71, 0.637);
+			border-radius: 50px;
+		}


Styles scrollbar in desktop mode. Feel free to edit style as you please

mhgamboa · 2021-12-10T19:53:24Z

src/ui/components/LeagueTopBar.tsx

+	useEffect(() => {
+		const leagueTopBar = leagueTopBarRef.current;
+		if (
+			!leagueTopBar ||
+			!show ||
+			leagueTopBar.scrollWidth <= leagueTopBar.clientWidth
+		)
+			return;
+
+		console.log("added");
+		leagueTopBar.addEventListener("wheel", handleWheel, { passive: false });
+
+		return () => {
+			console.log("removed");
+			leagueTopBar.removeEventListener("wheel", handleWheel, {
+				passive: false,
+			});
+		};
+	});
+
+	const handleWheel = useCallback(e => {
+		e.preventDefault();
+		const leagueTopBarPosition = leagueTopBarRef.current.scrollLeft;
+
+		leagueTopBarRef.current.scrollTo({
+			top: 0,
+			left: leagueTopBarPosition - e.deltaY + e.deltaX,
+			behaviour: "smooth",
+		});
+	}, []);
+


Enables scrolling for league-top-bar.

I added the event listener leagueTopBar.addEventListener("wheel", handleWheel, { passive: false }); as e.preventDefault() will not work with the react onwheel event without it

My one conern is that the useEffect() will run after every render, which may create multiple "wheel" event listeners. I wasn't actually to create this issue, but i left the two console.log() in for you to easily test if you can create the issue.

My one conern is that the useEffect() will run after every render, which may create multiple "wheel" event listeners. I wasn't actually to create this issue, but i left the two console.log() in for you to easily test if you can create the issue.

If you add a dependency array (2nd arg) of [handleWheel, show] to that useEffect, it will run many fewer times.

mhgamboa · 2021-12-10T20:00:47Z

src/ui/components/LeagueTopBar.tsx

-			className="league-top-bar flex-shrink-0 d-flex justify-content-end overflow-hidden mt-2"
+			className="league-top-bar flex-shrink-0 d-flex overflow-auto flex-row-reverse pr-3 pb-1 mt-2"


Fixes the alignment for the leageTopBar.

Apparently there is a bug where the css properties of overflow: auto; and justify-content: end combined make the scrollbar disappear.

To fix this this I used flex-row-reverse to align all of the items on the right side of the page combined with the games2.unshift(game); to correct the rendering order of games2 get around the issue.

pb-1 and pr-3 are used to give space between the scrollbar and the toggle button respectively

mhgamboa · 2021-12-11T00:48:26Z

src/ui/components/LeagueTopBar.tsx

 	}

 	const transition = { duration: 0.2, type: "tween" };

 	return (
 		<div
-			className="league-top-bar flex-shrink-0 d-flex justify-content-end overflow-hidden mt-2"
+			className="league-top-bar flex-shrink-0 d-flex overflow-auto flex-row-reverse pr-3 pl-1 pb-1 mt-2"


Also added pl-1 to give some space on the left side when scrolled all the way to the left end

dumbmatter · 2021-12-11T16:00:50Z

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 · 2021-12-13T16:51:33Z

src/ui/components/LeagueTopBar.tsx

+		if (
+			!leagueTopBar ||
+			!show ||
+			leagueTopBar.scrollWidth <= leagueTopBar.clientWidth


What's this check for?

!leagueTopBar ensures that leagueTopBarRef.current actually exists. On the initial render it doesn't exist for some reason (At least in my testing)

!show is used to make sure the useEffect only runs when the leagueTopBar exists. (No point in adding an event listener if the topBar isn't visible)

leagueTopBar.scrollWidth <= leagueTopBar.clientWidth is used to ensure that scrolling will remain normal when there is no overflow present. I put this line of code in because I couldn't scroll down when there was only one or two games and my mouse was hovering over the leagueTopBar

Okay, the scrollWidth/clientWidth check should now go inside the handleWheel, since useEffect will no longer be run every render.

dumbmatter · 2021-12-13T16:55:48Z

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 · 2021-12-13T20:37:57Z

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 · 2021-12-13T20:45:17Z

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 · 2021-12-15T00:57:00Z

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 · 2021-12-15T03:52:33Z

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 · 2021-12-15T15:56:57Z

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 · 2021-12-16T02:27:49Z

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 · 2021-12-16T03:51:19Z

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 · 2021-12-16T05:05:00Z

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 · 2021-12-16T15:55:55Z

Wow! Very interesting. I'm excited to see how it all turns out 🙌

Make scoreboxes scrollable (zengm-games#301)

bd54ea5

dumbmatter reviewed Dec 7, 2021

View reviewed changes

Fix UI to make scoreboxes scrollable

0363149

mhgamboa commented Dec 10, 2021

View reviewed changes

Add padding left to leaguetopbar

34b9fbd

mhgamboa commented Dec 11, 2021

View reviewed changes

dumbmatter reviewed Dec 13, 2021

View reviewed changes

Add dependencies to leageTopBar useEffect

d7d3a72

Fix useEffect Dependencies

5915454

dumbmatter merged commit c0ab04c into zengm-games:master Dec 16, 2021

dumbmatter mentioned this pull request Dec 16, 2021

Scrollable Top Bar #317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scoreboxes scrollable (#301) #400

Make scoreboxes scrollable (#301) #400

mhgamboa commented Dec 7, 2021

dumbmatter Dec 7, 2021

dumbmatter commented Dec 7, 2021 •

edited

Loading

mhgamboa commented Dec 7, 2021

mhgamboa commented Dec 10, 2021

mhgamboa Dec 10, 2021

mhgamboa Dec 10, 2021 •

edited

Loading

dumbmatter Dec 13, 2021

mhgamboa Dec 13, 2021

mhgamboa Dec 10, 2021

mhgamboa Dec 10, 2021

mhgamboa Dec 11, 2021

dumbmatter commented Dec 11, 2021

dumbmatter Dec 13, 2021

mhgamboa Dec 13, 2021

dumbmatter Dec 13, 2021

dumbmatter commented Dec 13, 2021

mhgamboa commented Dec 13, 2021

dumbmatter commented Dec 13, 2021

mhgamboa commented Dec 15, 2021

dumbmatter commented Dec 15, 2021

dumbmatter commented Dec 15, 2021

mhgamboa commented Dec 16, 2021

dumbmatter commented Dec 16, 2021

dumbmatter commented Dec 16, 2021

mhgamboa commented Dec 16, 2021

		className="league-top-bar flex-shrink-0 d-flex justify-content-end overflow-hidden mt-2"
		className="league-top-bar flex-shrink-0 d-flex overflow-auto flex-row-reverse pr-3 pb-1 mt-2"

Make scoreboxes scrollable (#301) #400

Make scoreboxes scrollable (#301) #400

Conversation

mhgamboa commented Dec 7, 2021

Choose a reason for hiding this comment

dumbmatter commented Dec 7, 2021 • edited Loading

mhgamboa commented Dec 7, 2021

mhgamboa commented Dec 10, 2021

Choose a reason for hiding this comment

mhgamboa Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbmatter commented Dec 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbmatter commented Dec 13, 2021

mhgamboa commented Dec 13, 2021

dumbmatter commented Dec 13, 2021

mhgamboa commented Dec 15, 2021

dumbmatter commented Dec 15, 2021

dumbmatter commented Dec 15, 2021

mhgamboa commented Dec 16, 2021

dumbmatter commented Dec 16, 2021

dumbmatter commented Dec 16, 2021

mhgamboa commented Dec 16, 2021

dumbmatter commented Dec 7, 2021 •

edited

Loading

mhgamboa Dec 10, 2021 •

edited

Loading