Skip to content

Update Tic Tac Toe Command#595

Merged
kosayoda merged 3 commits into
python-discord:mainfrom
Shivansh-007:fix/ttt
Jun 7, 2021
Merged

Update Tic Tac Toe Command#595
kosayoda merged 3 commits into
python-discord:mainfrom
Shivansh-007:fix/ttt

Conversation

@Shivansh-007
Copy link
Copy Markdown
Contributor

@Shivansh-007 Shivansh-007 commented Feb 16, 2021

Description

  1. Update x and o emoji in constants.Emojis to use x and o instead of the letters x and o
  2. Use embeds when you ask for board of a particular game, since it was pinging the user earlier. (.ttt log show {game_id})

Reasoning

Was approved by @Akarys42

Screenshots

Screenshot from 2021-03-15 16-05-09

Screenshot from 2021-03-15 16-05-20

Did you:

@RohanJnr
Copy link
Copy Markdown
Contributor

is it possible to have the X and O to be of different colors? or is single color fine?

@Shivansh-007
Copy link
Copy Markdown
Contributor Author

is it possible to have the X and O to be of different colors? or is single color fine?

Hmm, there are no other colour of x and o. So i thought just to use this, since it is clearly differentiable.

@HassanAbouelela HassanAbouelela added area: frontend Related to output and formatting priority: 2 - normal type: enhancement Changes or improvements to existing features labels Feb 18, 2021
Copy link
Copy Markdown
Contributor

@HassanAbouelela HassanAbouelela left a comment

Choose a reason for hiding this comment

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

Besides the change I requested, you have my tentative approval on the code.

In terms of actual functionality though, was the change in the x and o styles bikeshed? I don't particularly like the new ones, especially on light mode (you can compare the old and new styles in the screenshot below).

image

Comment thread bot/exts/evergreen/tic_tac_toe.py Outdated
@Xithrius Xithrius added the status: waiting for author Waiting for author to address a review or respond to a comment label Feb 21, 2021
@Xithrius Xithrius added status: needs review Author is waiting for someone to review and approve and removed priority: 2 - normal status: waiting for author Waiting for author to address a review or respond to a comment labels Feb 28, 2021
Base automatically changed from master to main March 13, 2021 20:09
@HassanAbouelela
Copy link
Copy Markdown
Contributor

This PR has been sitting here for many months now. All it does now is change one emoji for another, amongst the changes to the result. The changes to the result are fine, but I don't like the new emojis. Ultimately, that is a matter of preference, and I don't see how you could change that. I'll dismiss my review because while I don't agree with these changes, I won't block them either.

But here's the deal, let's not make minimal style changes like these anymore without at least discussing them, and coming to a consensus first, otherwise, we end up in this neverending pending state.

@HassanAbouelela HassanAbouelela dismissed their stale review May 15, 2021 06:06

See my comment above.

Copy link
Copy Markdown
Contributor

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

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

I see no point in letting this sit further, so I'll be merging this. I wholeheartedly agree with Scale, so keep his points in mind for future PRs.

@kosayoda kosayoda merged commit 0a6a355 into python-discord:main Jun 7, 2021
@Shivansh-007 Shivansh-007 deleted the fix/ttt branch June 7, 2021 09:41
@Xithrius Xithrius removed the status: needs review Author is waiting for someone to review and approve label Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: frontend Related to output and formatting type: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants