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.

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

Refactored DeckPickerWidget to handle empty state visibility. #17041

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Refactored DeckPickerWidget to handle empty state visibility. #17041

merged 2 commits into from
Oct 3, 2024
+127 −41

Conversation

xenonnn4w
Copy link
Contributor

@xenonnn4w xenonnn4w commented Sep 8, 2024 •
edited
Loading

Purpose / Description

Handle empty state visibility, allowing users to reconfigure the widget by clicking the empty state message in the Deck Picker Widget.

Approach

Handled similar to that Card Analysis Widget.

How Has This Been Tested?

image

Hiid.mp4

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

@david-allison
Copy link
Member

What does this look like?

@xenonnn4w
Copy link
Contributor Author

added in the PR description.

@xenonnn4w
Copy link
Contributor Author

I reviewed the code and noticed an issue with the visibility of empty_deck. I plan to investigate and resolve it when I have more time.

@xenonnn4w
Copy link
Contributor Author

Fixed!

mikehardy
mikehardy requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@mikehardy mikehardy 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 good - after looking through our back stack management though, I'm curious if we are missing a CLEAR_TOP on the Intent for the reviewer though

Otherwise (that is: the bulk of the PR): looks good

AnkiDroid/src/main/java/com/ichi2/widget/deckpicker/DeckPickerWidget.kt Outdated Show resolved Hide resolved
@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Sep 16, 2024
@xenonnn4w xenonnn4w added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Sep 16, 2024
mikehardy
mikehardy approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Fantastic, I think this looks pretty good and from the video it looks pretty good to me

If you are curious at all - and I mention it because I have seen you do related work so I think you might be interested? - you might look at #17073 to consolidate our usage of these flags...

@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Sep 16, 2024
@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Sep 29, 2024
@xenonnn4w
Refactored DeckPickerWidget to handle empty state visibility, allow…
d562a59 
…ing users to reconfigure the widget by clicking the empty state message.
@xenonnn4w xenonnn4w force-pushed the update_deck_picker_widget branch 2 times, most recently from b5d9633 to 451935b Compare October 2, 2024 20:51
@xenonnn4w xenonnn4w removed Has Conflicts Needs Author Reply Waiting for a reply from the original author labels Oct 2, 2024
Arthur-Milchior
Arthur-Milchior reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

Is this issue specific to deck picker widget?
I'd have expected it could be the same issue in your card analysis widget, given that it also requires a deck.

If so, I'd appreciate if you could have change both widgets at once, idially without copy pasting too much code

AnkiDroid/src/main/res/values/08-widget.xml Outdated Show resolved Hide resolved
AnkiDroid/src/main/res/values/08-widget.xml Outdated Show resolved Hide resolved
AnkiDroid/src/main/res/values/08-widget.xml Outdated Show resolved Hide resolved
@xenonnn4w
Copy link
Contributor Author

Will create a separate PR for card analysis widget if the changes regarding the deck picker widget get's confirmed regarding this issue.

@xenonnn4w
Copy link
Contributor Author

Updated preview in How Has This Been Tested?.

lukstbit
lukstbit approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Review Needs Second Approval Has one approval, one more approval to merge labels Oct 3, 2024
@lukstbit lukstbit added this pull request to the merge queue Oct 3, 2024
Merged via the queue into ankidroid:main with commit 6151c32 Oct 3, 2024
9 checks passed
@github-actions github-actions bot added this to the 2.19 release milestone Oct 3, 2024
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 3, 2024
@lukstbit lukstbit mentioned this pull request Oct 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arthur-Milchior Arthur-Milchior Arthur-Milchior left review comments

@mikehardy mikehardy mikehardy approved these changes

@lukstbit lukstbit lukstbit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.19 release
Development

Successfully merging this pull request may close these issues.
5 participants
@xenonnn4w @david-allison @Arthur-Milchior @mikehardy @lukstbit