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

fix: Use androidx.appcompat for better AlertDialog styling #93

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

Conversation

nikclayton
Copy link
Contributor

Bumps minSdk to 21.

Remove unnecessary API dependency on Kotlin stdlib.
The user might register with a distributor, and then delete the
distributor app.

If that happens the user is not prompted if there are no distributors,
and re-registration does not happen.

Fix this by checking the ack'ed distributor is still in the list of
installed distributors.
Previous code ignored the dialog's default layout and set its own
textview. I think this was so a LinkMovementMethod could be set on the
message.

This meant the view didn't follow normal Android dialog dimensions
(e.g., padding, margins, text layout), and looked out of place.

Fix this. Use the normal AlertDialog message. Set an onShowListener
that sets the LinkMovementMethod when the dialog is shown.
The strings used in default dialogs were hardcoded. This meant they
couldn't be translated, or use the default platform strings (e.g.,
android.R.string.ok).

Fix that. Convert `RegistrationDialogContent`, `NoDistributorDialog`,
and `ChooseDialog` to interfaces, and provide implementations of those
interfaces that
@p1gp1g
Copy link
Member

p1gp1g commented Aug 11, 2024

I think this is preferable to not update the minSdk

@nikclayton
Copy link
Contributor Author

Would you consider splitting this in to android-connector and android-connector-ui libraries, with the "...WithDialog" related functionality moving to the -ui library?

Looking at the first and third screenshot in #87, the difference in visuals is pretty jarring, especially when this dialog might be one of the first bits of UI they see from an app that uses this library (assuming that apps try and register early in the startup process).

@p1gp1g
Copy link
Member

p1gp1g commented Aug 12, 2024

You mean publishing a new library ?

The purpose of this dialog is to provide a quick and dirty way to use the lib, but most app write their own way to pick a distributor. So I don't think it is worth losing 5 SDK versions

@nikclayton
Copy link
Contributor Author

You mean publishing a new library ?

Yes. The -ui library would have minSdk set to 21 to allow the use of androidx.appcompat, the existing library would stay at 16.

The purpose of this dialog is to provide a quick and dirty way to use the lib, but most app write their own way to pick a distributor. So I don't think it is worth losing 5 SDK versions

https://github.com/search?q=UnifiedPush+registerAppWithDialog&type=code has some false positives, but I'm not sure it supports the idea that "most apps write their own way".

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