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

Add proposal for migrating AlertDialog #784

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

Conversation

Abbondanzo
Copy link

Migrate to androidx.appcompat.app.AlertDialog

This proposal suggests migrating all existing uses of android.app.AlertDialog to androidx.appcompat.app.AlertDialog to add support library features. Currently, the two primary uses are in the AlertFragment and DevSupportManagerBase classes.

@Abbondanzo
Copy link
Author

Some past works include:
facebook/react-native#29832
facebook/react-native#13184

@Abbondanzo
Copy link
Author

Abbondanzo commented May 8, 2024

Proof-of-concept:
facebook/react-native#44495

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just left minor questions to clarify the text


## Adoption strategy

This introduces a subtle breaking change in the form of theme support changes. Theming is now applied via `alertDialogTheme`, not `android:alertDialogTheme`. We would have to include a blurb in the release about the theme changes and dropping support for `android.app.AlertDialog`.
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the impact of this change will be minimal. I expect most of the brownfield users to use AppCompatActivity already.

Copy link
Member

Choose a reason for hiding this comment

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

Also cc @javache as we discussed about Activity vs AppCompatActivity

Add a release versioning strategy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants