Skip to content

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

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
Merged
merged 19 commits into from
Nov 20, 2024
Merged

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

merged 19 commits into from
Nov 20, 2024

Conversation

@Jimima
Copy link
Contributor

@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.

@Jimima Jimima marked this pull request as draft November 6, 2024 15:57
@Jimima
Copy link
Contributor 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
coordinates: true,
pieceAnimation: true,
showMaterialDifference: true,
clockPosition: ClockPosition.left,
Copy link
Collaborator

@tom-anders tom-anders Nov 9, 2024

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
Contributor Author

@Jimima Jimima Nov 9, 2024

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
Contributor Author

@Jimima Jimima Nov 11, 2024

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
Collaborator

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
Contributor 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
Collaborator

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
Contributor Author

Jimima commented Nov 11, 2024 •
edited
Loading

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

@Jimima Jimima force-pushed the 1098-feature-change-clock-position branch from e86e9b7 to 4c72000 Compare November 20, 2024 09:32
@veloce veloce merged commit 98c19bd into lichess-org:main Nov 20, 2024
1 check passed
@Jimima
Copy link
Contributor Author

Jimima commented Nov 20, 2024

😁 thanks!

@Jimima Jimima mentioned this pull request Nov 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@veloce veloce veloce approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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