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

Mark annulled games more clearly. #1245

Merged
merged 4 commits into from Oct 26, 2020
Merged

Mark annulled games more clearly. #1245

merged 4 commits into from Oct 26, 2020

Conversation

benjaminpjones
Copy link
Contributor

@benjaminpjones benjaminpjones commented Oct 25, 2020

Fixes #1244

Proposed Changes

  • In the Game view, strikethrough the game result and add "GAME ANNULLED"
  • Strikethrough the game result in the Game History pane if the game has been annulled.

Screen Shot 2020-10-24 at 11 42 43 PM

Screen Shot 2020-10-24 at 11 42 23 PM

@@ -29,13 +29,11 @@ import {PlayerIcon} from 'PlayerIcon';
import {GameList} from "GameList";
import {Player} from "Player";
import * as preferences from "preferences";
import {updateDup, alertModerator, getGameResultText, ignore} from "misc";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think you meant to remove the alertModerator import here? it wasn't related to displaying the endstate of the game

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that it is unrelated, but alertModerator is an unused import (courtesy of my linter). I'm guessing it fell off whenever that functionality was moved into PlayerDetails.

If you think I should remove that kind of stuff for better chance at being approved, I will, but I thought it was a good idea to tidy up while I was looking at that file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just caught my eye, I can't recommend one way or another... anoek may advise whether cleanup should have its own PR or not, frankly it's not my call I just wanted to see why it was included

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're curious, commit b88bc3369f17e5036d309ff96cf1e559b83bc2e1 is when alertModerator was removed from the profile page :)

Copy link
Member

@anoek anoek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!

src/views/Game/Game.tsx Outdated Show resolved Hide resolved
src/views/User/User.tsx Outdated Show resolved Hide resolved
@benjaminpjones
Copy link
Contributor Author

I think I fixed it, however I am not sure how to test translations:

Screen Shot 2020-10-26 at 11 56 24 AM

Screen Shot 2020-10-26 at 11 56 47 AM

I assume 'annulled' is already in the dictionary... does the context parameter for pgettext need to be more generic?

@anoek
Copy link
Member

anoek commented Oct 26, 2020

Yep "annulled" is already in there, as is "Game has been annulled", "Annulled", and "game annulled". So "Game Annulled" isn't in there yet, but close variations are. I would say if "Game has been annulled" looks good there (like it's not too big or doesn't look funny) lets go with that, otherwise we'll just add "Game Annulled" to the list and give the translators some more work to do :)

@anoek
Copy link
Member

anoek commented Oct 26, 2020

I guess for "annulled" just make that _("annulled") and you should see it translated, that's the variant that's already translated.

@benjaminpjones
Copy link
Contributor Author

Okay, I went ahead and submitted the change to 'annulled':
Screen Shot 2020-10-26 at 1 44 56 PM

I did not submit the change to "Game Annulled" because changing it to "Game has been annulled" did not translate when I tested in Spanish, and therefore (hopefully) not too much work to translate both in one go.

@anoek
Copy link
Member

anoek commented Oct 26, 2020

Awesome thanks!

@anoek anoek merged commit 02e083b into online-go:devel Oct 26, 2020
@GreenAsJade
Copy link
Contributor

Nice one!

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.

Annulled games not clearly marked in the UI
4 participants