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

Log visualize phases #243

Closed
wants to merge 5 commits into from
Closed

Log visualize phases #243

wants to merge 5 commits into from

Conversation

Stefan-Hanke
Copy link
Contributor

This PR implements a part of #227 - namely, visualize phases and changes thereof.
I'm no designer, but with this, the current phase and changes thereof are clearly visible.

image

Checklist

  • Use a separate branch in your local repo (not master).
  • Test coverage is 100% (or you have a story for why it's ok).

Note

Nicolo will be away for 2-3 weeks starting July 16th 2018, so expect some delays in getting a response.

@Stefan-Hanke
Copy link
Contributor Author

oh my - I relied too much on the prepush hook to catch errors. First prop type validation, and now uncovered lines. 👎

@coveralls
Copy link

coveralls commented Jul 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 94edef8 on Stefan-Hanke:log-visualize-phases into 52be2c3 on google:master.

@nicolodavis
Copy link
Member

nicolodavis commented Jul 26, 2018

Hmm, the colors get a bit jarring for longer logs.

I have an alternative proposal in https://github.com/google/boardgame.io/tree/log-phases that looks like the following (turns on the left and phases on the right; it also has the nice property of conveying to the user the fact that turns and phases are orthogonal concepts):
screenshot from 2018-07-26 23-41-07

What do you think?

@Stefan-Hanke
Copy link
Contributor Author

Stefan-Hanke commented Jul 26, 2018

Looks ok to me.

@nicolodavis
Copy link
Member

Ok, I'll get that branch in and close this then.

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.

3 participants