-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Feature: Allow changing deck name in statistics screen #15648
Conversation
34f2925
to
9d2531b
Compare
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.
Is there a reason you didn't go with the spinner in the ActionBar
? I'd expect this UI to be at the top of the screen
Ohh that's because the deck name is already shown in the page why to have it at top would that be a good ui ? I did mention that in my pr template,
|
I feel this significantly reduces vertical space, which is what people want from this screen
|
9d2531b
to
555c1ea
Compare
AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckDropDownAdapter.kt
Outdated
Show resolved
Hide resolved
555c1ea
to
f996aaf
Compare
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.
Visually, this looks awesome, thank you!
AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckDropDownAdapter.kt
Outdated
Show resolved
Hide resolved
f996aaf
to
1ece7f9
Compare
efada90
to
bde8a9f
Compare
private fun changeDeck(selectedDeck: String) { | ||
val javascriptCode = """ | ||
var textBox = [].slice.call(document.getElementsByTagName('input'), 0).filter(x => x.type == "text")[0]; | ||
textBox.value = "deck:$selectedDeck"; | ||
textBox.dispatchEvent(new Event("input", { bubbles: true })); | ||
textBox.dispatchEvent(new Event("change")); | ||
""".trimIndent() | ||
webView.evaluateJavascript(javascriptCode, null) |
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.
if possible, add an AndroidTest to get any backend breakages
This likely will need more feedback rounds than I have availability for. Leaving to the other reviewers |
@BrayanDSO Is this ok? I made some changes |
31453a4
to
39f5818
Compare
39f5818
to
1821224
Compare
9574e50
to
545b02b
Compare
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.
Approve after Mike's comment is taken care of
1b40e43
to
d08e408
Compare
Note for me mainly as I look to merge all things merge-able and I scan this, only outstanding thread is this, hoping for a PR or issue to mark the code generating the form we twiddle here upstream so we get a note if they modify it: #15648 (comment) |
A Spinner was introduced in the statistics screen top bar which can be used by the user to change the current selected deck and update the WebView with the statistics for the new selected deck. Note: the deck selection mechanism is decoupled from the general DeckSpinnerSelection/DeckSelectionDialog system so changing the deck in the statistics screen doesn't modify the selected deck for other parts of the app.
d08e408
to
6bcc936
Compare
Current status: I think we're really close here. Only sticking point is cross-repo programmatic usage of the web form and that will resolve but it may take an iteration or so |
I think that this PR can be merged now as it is and the change from "first text box" to ID can be made in a subsequent PR. Reasoning: Obviously, this is just my opinion and the maintainers are free to disagree. |
That's a reasonable point @user1823 |
Purpose / Description
Reintroduce Deck Selector to the Statistics Page
Fixes
Approach
Use M3 button to allow user to change deck, not using the spinner here as the deck name is already displayed in the text field
How Has This Been Tested?
Google emulator API 34
Tested the orientation change too
test-stats.webm
Checklist
Please, go through these checks before submitting the PR.