feat: improve the review ahead dialog - #21709
Conversation
david-allison
added
the
Needs Second Approval
|
Important Maintainers: This PR contains Strings changes
|
|
@ZornHadNoChoice if you have feedback, it'd be very well-received! |
|
The title "Review ahead" is bold instead of being big like other dialogs. |
david-allison
added
the
Needs Author Reply
|
Holdover from the original change, I thought it was intentional. Thanks!! |
Title: "Review ahead" Description: "Review cards due in the next:" Positive button: "Create" Part of 20600 Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com> Assisted-by: Claude Fable 5.1 [rebase and commit split]
Part of 20600 Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com> Assisted-by: Claude Fable 5.1 [rebase and commit split]
Unsigned, at most 5 digits and no leading zeros, reusing NonLeadingZeroInputFilter from 20520. Part of 20600 Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com> Assisted-by: Claude Fable 5.1 [rebase and commit split]
Search the deck as the user types and show an inline error with the button disabled, instead of an error dialog after submission. Part of 20600 Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com> Assisted-by: Claude Fable 5.1
Matches the number dialogs from 20520: below the minimum shows "Minimum value is 1", while an empty field only disables 'Create'. Part of 20600 Assisted-by: Claude Fable 5.1
The stored default may match no cards; without this, tapping 'Create' immediately still produced the error dialog. Fixes 20600 Assisted-by: Claude Fable 5.1
6ca4b73 to
8ac6e61
Compare
|
david-allison
removed
the
Needs Author Reply
lukstbit
left a comment
lukstbit
left a comment
There was a problem hiding this comment.
LGTM, with the note below on wording.
| if (contextMenuOption == STUDY_TAGS) { | ||
| TR.sentenceCase.chooseTags | ||
| } else if (contextMenuOption == STUDY_AHEAD) { | ||
| getString(R.string.dialog_positive_create) |
There was a problem hiding this comment.
Using the previous "Ok" label reads better IMO:
Review cards due in the next 5 days? => Ok // reads more natural
Review cards due in the next 5 days? => Create // I understand we create a custom study session but feels out of place
Even if we keep the "Create" label we become inconsistent: Review ahead and Review forgotten cards have issues to be changed to "Create" while the options Preview new cards, Increase today's new card limit and Increase today's review card limit will use "Ok"(and for all 3 options it's the same discussion as above).
|
I rushed a bit, something that is not that ok: this dialog uses a title while all other options do not use a title. |
lukstbit
added
Needs Author Reply
|
@lukstbit Zorn specifically asked for a title, would you want me to split out this change so we can discuss adding titles to each of the dialogs? |
Let's merge this as it is now and then create an issue to discuss about the consistency of all the dialogs under custom study, title/positive button label. |
david-allison
added
the
Needs reviewer reply
|
I could quickly knock out the title change, but we're too close to a release window, so we'd be inconsistent. I'll split it out for later Will wait for @ZornHadNoChoice on the other thoughts. This isn't a dialog I use |
|
In my original issue, I suggested removing the Preview new cards, Increase today's new card limit and Increase today's review card limit dialogs completely. That's why I didn't make sub-issues for them. The other 3 dialogs have issues where I suggested having a title and renaming the button to "Create". |
4 participants
Note
Assisted-by: Claude Fable 5.1
This PR is a fixup of:
Purpose / Description
Fixes
Approach
Fixup the linked PR, then add my own changes and improvements.
I initially refactored the
debouncecode into a class, but after a manual test, 300ms was far too much latency and the DB calls were effectively instant.How Has This Been Tested?
SDK 37 Google Pixel 9 Pro
Checklist