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.

Sign up for GitHub

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

Jump to bottom

Added choice picker in settings and game settings sheet to switch the clock position to the left side #1136

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Added choice picker in settings and game settings sheet to switch the clock position to the left side #1136

wants to merge 15 commits into from

Conversation

Jimima
Copy link

@Jimima Jimima commented Nov 5, 2024

Related to issue #1098

New developer here, I have had a crack at this. If there are omissions or errors or anything I have done incorrectly please just let me know. I am happy to learn more 😄

veloce
veloce reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@veloce veloce left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Just a couple of comments, otherwise it looks good.

ios/Podfile.lock Outdated Show resolved Hide resolved
lib/src/model/settings/board_preferences.dart Outdated Show resolved Hide resolved
lib/src/view/game/game_settings.dart Outdated Show resolved Hide resolved
lib/src/view/game/game_settings.dart Show resolved Hide resolved
@Jimima Jimima marked this pull request as draft November 6, 2024 15:57
@Jimima
Copy link
Author

Jimima commented Nov 6, 2024

Made the changes discussed. I honestly stole a bit from the theme selection implementation so I hope the same approach makes sense in this context. Comments are very welcome of course!

@Jimima Jimima marked this pull request as ready for review November 6, 2024 16:50
@Jimima Jimima requested a review from veloce November 6, 2024 16:55
@Jimima Jimima changed the title Added toggle in settings and game settings sheet to switch the clock position to the left side Added choice picker in settings and game settings sheet to switch the clock position to the left side Nov 6, 2024
tom-anders
tom-anders reviewed Nov 9, 2024
View reviewed changes
lib/src/model/settings/board_preferences.dart Outdated
@@ -132,6 +139,7 @@ class BoardPrefs with _$BoardPrefs implements Serializable {
coordinates: true,
pieceAnimation: true,
showMaterialDifference: true,
clockPosition: ClockPosition.left,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't right be the default to match the current UI?

Copy link
Author

Choose a reason for hiding this comment

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

Yes typo will fix

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@tom-anders
Copy link
Contributor

tom-anders commented Nov 9, 2024 •
edited
Loading

Currently, when the clock is set to left, there's some space on the right of the player widget (see screenshot below).

I think you can fix this by adding mainAxisAlignment: clockPosition == ClockPosition.right ? MainAxisAlignment.start : MainAxisAlignment.end to the two Row widgets inside the playeWidget

Screenshot_1731145252

@Jimima
Copy link
Author

Jimima commented Nov 11, 2024

@tom-anders I added the snippet to the Row that I think needs it, I am not sure if the other one does. This gives the following layout:

This looks like what you were suggesting but let me know if it isn't 🙂

@tom-anders
Copy link
Contributor

tom-anders commented Nov 11, 2024 •
edited
Loading

Yeah looks good! But I think you also need it to the Row that displays the material difference below the user name (see my original screenshot, you can capture some pieces to check this)

@Jimima
Copy link
Author

Jimima commented Nov 11, 2024 •
edited
Loading

Yes I see, makes sense I should have made some captures! Now added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-anders tom-anders tom-anders left review comments

@veloce veloce Awaiting requested review from veloce

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jimima @tom-anders @veloce