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

feat(new reviewer): tablet layout #17941

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

Conversation

BrayanDSO
Copy link
Member

@BrayanDSO BrayanDSO commented Feb 8, 2025

Purpose / Description

Improve the new reviewer style in tablets and make it more similar to Anki

Approach

Move the toolbar to the bottom of the screen and put the answer buttons there

How Has This Been Tested?

Emulator 35 in a lot of scenarios with the new reviewer settings (hide system bars; ignore display cutout; frame style; hide answer buttons; menu)

Screenshot_20250209_153607_AnkiDroid
Screenshot_20250209_153633_AnkiDroid
Screenshot_20250209_153731_AnkiDroid
Screenshot_20250209_153716_AnkiDroid
Screenshot_20250209_153813_AnkiDroid
Screenshot_20250209_153628_AnkiDroid
Screenshot_20250209_153643_AnkiDroid

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@BrayanDSO BrayanDSO added Needs Review Blocked by dependency Currently blocked by some other dependent / related change labels Feb 8, 2025
@david-allison

This comment was marked as resolved.

@BrayanDSO

This comment was marked as resolved.

@david-allison
Copy link
Member

[comment/non-blocking]

Is there any way that a 'smoke test' can be added here [load the UI in the emulated tests]?

Having two layouts makes it much more likely that an edit to one layout will break the other

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I think this looks unusual with the counts at the bottom

But, users will get used to it, it's just unfamiliarity on my behalf

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Feb 8, 2025
@BrayanDSO
Copy link
Member Author

BrayanDSO commented Feb 8, 2025

Is there any way that a 'smoke test' can be added here [load the UI in the emulated tests]?

Done.

I think this looks unusual with the counts at the bottom

Anki/AnkiMobile has them at the bottom center. So I don't think that will be very unusual. Haven't put it at the bottom center because that either needs more vertical space (like in Anki desktop) or removes the Show answer test, which makes things less intuitive IMO (like in AnkiMobile). Also, leaving at the left makes the toolbar more symmetrical

@david-allison
Copy link
Member

david-allison commented Feb 8, 2025

I think it's just the feeling of "it's new".

Mild concern about misclicks on the undo button and the lack of symmetry if more icons are added (could they be on both the left & right?)

But... it's forward progress. Let's do this

@BrayanDSO
Copy link
Member Author

There's the alternative of making the menu size fixed and centralize the buttons instead of extending them, so it would be always symmetrical

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Feb 8, 2025
@BrayanDSO BrayanDSO marked this pull request as draft February 8, 2025 22:43
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Feb 9, 2025
@BrayanDSO BrayanDSO marked this pull request as ready for review February 9, 2025 15:06
@BrayanDSO
Copy link
Member Author

BrayanDSO commented Feb 9, 2025

  1. The menu width is now restricted to two actions (more can still be scrolled) + the overflow menu, and it's quite symmetrical in that scenario.
  2. If less options are used, the empty space is consumed (it looked better that way IMO)
  3. If Hide answer buttons is set to true, the menu is expanded.
  4. Also had to convert everything to a ConstraintLayout so I could avoid as much as possible doing stuff programatically

@BrayanDSO BrayanDSO removed the Blocked by dependency Currently blocked by some other dependent / related change label Feb 9, 2025
@BrayanDSO BrayanDSO force-pushed the tablet-style branch 2 times, most recently from 2b0af6a to 5561612 Compare February 9, 2025 16:59
@BrayanDSO BrayanDSO marked this pull request as draft February 9, 2025 18:23
@BrayanDSO BrayanDSO marked this pull request as ready for review February 9, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Second Approval Has one approval, one more approval to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants