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

Replaced CustomStudyListener with FragmentResultApi in CustomStudyDialog #17692

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

Conversation

Aditya13s
Copy link
Contributor

Purpose / Description

Replace CustomStudyListener with the FragmentResultApi in CustomStudyDialog to improve fragment communication.

Fixes

Approach

Replaces the CustomStudyListener functions with setFragmentResult to pass CustomStudyAction inside a bundle. The result is then listened inside DeckPicker using supportFragmentManager.setFragmentResultListener.

How Has This Been Tested?

Tested on Android 12

Checklist

  • 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

@Aditya13s
Copy link
Contributor Author

@david-allison Kindly review and advise if any additional changes are required

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.

  • define CustomStudyAction.fromOrdinal
  • Extract the key to a constant
    • Consider extracting the bundle arg key to a constant
  • 🐞 This doesn't handle all callers
  • Remove the CustomStudyListener

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Dec 30, 2024
@Aditya13s
Copy link
Contributor Author

Aditya13s commented Dec 31, 2024

Remove the CustomStudyListener

@david-allison
CustomStudyListener Interface or only the val ?

@david-allison
Copy link
Member

The interface, it should no longer be necessary if you're using the result API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Conflicts Needs Author Reply Waiting for a reply from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom study option raises an exception
2 participants