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

Add pass to conditional planner #2713

Merged
merged 1 commit into from
Jun 15, 2024
Merged

Conversation

LForchini
Copy link
Contributor

Fixes #251

Proposed Changes

  • Add pass to the conditional move planner

Is there a nicer way to localise the pass message than the check I used? I considered having the prettyCoord function return undefined when given invalid moves instead of the string pass.

I'd also like some feedback on the UI here:
sc

Copy link

github-actions bot commented Jun 11, 2024

Uffizzi Preview deployment-52912 was deleted.

@benjaminpjones
Copy link
Contributor

Looks good to me!

I considered having the prettyCoord function return undefined when given invalid moves instead of the string pass.

I wouldn't change it, or at least I think the current behavior (and the way you handle it) is okay. It doesn't make a huge difference what we return in the "invalid" case, but at least now it prints out something reasonable in cases where we haven't explicitly handled it. E.g. the Joseki tool

Screenshot 2024-06-11 at 5 56 50 PM

I'd also like some feedback on the UI here

This one's tough, I can't think of anything better than this. Maybe other people will have feedback...

@BHydden
Copy link
Collaborator

BHydden commented Jun 12, 2024

I think it looks tidy. OGS is renowned for having noone on the team especially gifted at UI/UX 😅 so tidy and consistent seem adequate goals to aim for...

@LForchini LForchini marked this pull request as ready for review June 12, 2024 09:09
@anoek
Copy link
Member

anoek commented Jun 15, 2024

Thanks!

@anoek anoek merged commit 422ebf2 into online-go:devel Jun 15, 2024
4 checks passed
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.

Cannot pass in Conditional Move Planner
4 participants