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

Advance through all active games #852

Merged
merged 19 commits into from Sep 6, 2019

Conversation

@BHydden
Copy link
Contributor

commented Aug 4, 2019

This is the start of my attempt to implement #851

However, my efforts have had the opposite effect... instead of adding cycle active games, it seems the notification doesn't show up at all anymore.

@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

I suspect line 314 is where I went wrong... probably should have paid closer attention when anoek said

delete it when game.phase === "finished"

Will look at tweaking that when I get back to it.

@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Seems to have returned to how it was before I touched it :P normal 'your move' notifications are working again but the new 'cycle active games' is still totally non-functioning ☹️

@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

discovered that the 'your move' notification shows up, but doesn't work the way it's meant to (clicking does nothing) so I'm not as close to square 0 as I thought :P #justkeephacking

BHydden added 5 commits Aug 16, 2019
fixed links, pool seems to correctly populate from either your move o…
…r active if former is empty

only task remaining is to fix the notification so that move counter displays 0 is pool is active not your move
@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

only task remaining is to fix the notification so that move counter displays 0 if pool is 'active' not 'your move'

BHydden added 6 commits Aug 16, 2019
Revert "cleanup"
remove changes moved over to branch altdev2
This reverts commit 509688c.
@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Clicking on the space where the "your move on x boards" icon should be always works now.
If it is your turn on >0 boards, clicking it will cycle through them.
If you have games but none of them are your turn, clicking it will cycle through all games.

...but for the life of me, the only way I can get a "0" in the notification is when I accidentally break something and then there's always a "0" there even if you have no games at all, and when I have it working properly I can't get the "0" notification to show :(

BHydden added 2 commits Sep 6, 2019
@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

very open to any ideas people might have about how I can get it to render correctly :P

@@ -454,7 +478,9 @@ export class TurnIndicator extends React.Component<{}, any> { /* {{{ */
constructor(props) {
super(props);
this.state = {
count: Object.keys(notification_manager.boards_to_move_on).length
count: Object.keys(notification_manager.boards_to_move_on).length,
total: Object.keys(notification_manager.active_boards).length,

This comment has been minimized.

Copy link
@flovo

flovo Sep 6, 2019

Contributor

You assign a value to total only in the constructor. At this point total is always 0.

For count it was done with:


this.event_emitter.emit("turn-count", Object.keys(this.boards_to_move_on).length);

notification_manager.event_emitter.on("turn-count", (ct) => {
this.setState({count: ct});
});

This comment has been minimized.

Copy link
@BHydden

BHydden Sep 6, 2019

Author Contributor

I don't think this is the case, it is still updating somehow (though don't ask me why) because the "it's your turn to move" counter still works properly. Sorry if I wasn't clear but that half still renders, the only thing the doesn't render is the 0 and the circle when you have active games but not any that are your turn...

So, visually it still looks exactly the same as it does on the live site... but functionally it works how I want it, if that makes sense?

This comment has been minimized.

Copy link
@flovo

flovo Sep 6, 2019

Contributor

when you change
<span className={this.state.total > 0 ? "active count" : "count"}><span>{this.state.count}</span></span>
to
<span className={this.state.total > 0 ? "active count" : "active count"}><span>{this.state.total}</span></span>

you can see that total is always 0.

This comment has been minimized.

Copy link
@BHydden

BHydden Sep 6, 2019

Author Contributor

ok yes I see that you're right about this point and I will fix it now, but I can't see how it would cause the specific rendering issues I'm having... 😕

This comment has been minimized.

Copy link
@BHydden

BHydden Sep 6, 2019

Author Contributor

well I still have no idea why, but that magically fixed everything :D haha it's now working perfectly 😆

Thanks a lot @flovo ❤️

@@ -304,6 +321,9 @@ class NotificationManager {
});
comm_socket.on("active_game", (game) => {
delete this.boards_to_move_on[game.id];
if (game.phase === "finished") {
delete this.active_boards[game.id];

This comment has been minimized.

Copy link
@flovo

flovo Sep 6, 2019

Contributor

I would remove the if and leave the delete to always delete this.active_boards[game.id]; It seems more consistent with what we do for boards_to_move_on. If the game is supposed to be in active_boards, we readd it anyway.

This comment has been minimized.

Copy link
@BHydden

BHydden Sep 6, 2019

Author Contributor

I had the same feeling, but in the forum thread anoek advised to write it this way, and this part of the code is working as desired. The only part that is currently not working properly is rendering "0 moves indicator" while total games > 0 and boards_to_move_on = 0 .

BHydden added 2 commits Sep 6, 2019
@BHydden

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@anoek thanks to some help from @flovo I think this is finally ready to ship :)

@anoek anoek removed the work in progress label Sep 6, 2019

@anoek

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Awesome, thanks @BHydden and @flovo :)

@anoek anoek merged commit 72ad4a9 into online-go:devel Sep 6, 2019

1 check passed

Shippable Run 1424 status is SUCCESS.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.