-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Enable deck option target selection in study screen #19963
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
base: main
Are you sure you want to change the base?
Conversation
|
Awesome! Something feels like it's missing visually, but I don't know what What happens when a deck in the list is long and truncation occurs? Does What if there's a lot of entries in the list? I'd be tempted to add the card count as a subtitle and see how this looks Can you colour the filtered deck options the same colour as in the deck picker? Does cancel need to be there? Back or tapping outside should be sufficient |
|
I might not be seeing clearly, but the dialog title doesn't feel prominent enough. Also, the Material 2 guidelines agree with david-allison. |
The implementation is inserted around the current code so other usages outside of the new study screen work as before. See https://github.com/lukstbit/anki/blob/d24d2e33943af2361b5a9880572b30887efcf3ee/qt/aqt/deckoptions.py#L83-L100
354ef23 to
87279d8
Compare
|
I updated the images.
Just wraps around and makes the row bigger. I think this is better than truncating.
At most there could be 3 entries based on the implementation.
I don't think this is a good idea: it would complicate the implementation and I'm not sure what value offers to the user(besides desktop doesn't show this).
Should've done this from the start.
No and I removed it. I added it initially to follow the desktop ui. I updated the dialog appearance by using MaterialAlertDialogBuilder. It has the same surface/container colors issue but this will be fixed later. The contrast for dynamic color on black theme is not great but I'm tempted to leave it for now and wait for the surface colors to be addressed. Note on the previous design: I just used the normal dialog. Having the title so long probably feels like a message text and not a title. |
| /** | ||
| * True if we need to show to the user a list of decks before going to the deck options, false otherwise. | ||
| */ | ||
| val DeckOptionsDestination.haMultipleOptions: Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ha -> has
Also, you could have this property inside the class instead of being a extension function on the same file. That way, we don't need to make an extra import on other packages
Purpose / Description
Added an implementation for this feature around the current code of DeckOptionsDestination. I'm open for suggestions on UI, I just added a simple dialog:
Normal decks(and with long names) + filtered decks
Notice: the contrast isn't great for filtered deck color and black background.
Fixes
How Has This Been Tested?
Opened a lot of decks options screens, verified other usages of DeckOptionsDestination, ran tests.
Checklist