Skip to content

Update dependencies and add textWidget parameter to SignInWithAppleButton #462

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

IndigoSoftwares21
Copy link

Description:

  • Updated project dependencies to their latest compatible versions.
  • Added a new textWidget parameter to the SignInWithAppleButton component, allowing custom text widgets instead of the default label.
  • Ensured backward compatibility by maintaining the default behavior when textWidget is not provided.

@tp
Copy link
Collaborator

tp commented Mar 18, 2025

@IndigoSoftwares21 Thanks for the suggestion.

I think we made the text a String (and not a Widget) on purpose, as the font and its size needed to be in relation the button height and icon size (at least in Apple's guidelines as I recall them).

But over the years I have definitely also seen more custom SiwA buttons, like some using the app's font across all social logins (and not the brand-specific / default one per button).

Could you quickly state you use-case, just to make sure we're solving the right thing here?

(Also the lock files could go into another PR or be excluded. As long as we don't change the ranges in the pubspec.yaml files the consumer of the package would not see a difference - but this might affect CI inadvertently, at least we must re-check with the lowest version there.)

@IndigoSoftwares21
Copy link
Author

Hello @tp thank you for your response, I used a text widget, because the current font style and sizing did not match my app's style/theme.

We can leave a comment in the docs to let user's know the recommended font size.

I will exclude my changes to the pubspec.yaml and pubspec.lock files

@tp
Copy link
Collaborator

tp commented Mar 20, 2025

Thanks. This is fine with me.

I wonder how we don't offer a localized version of the "Sign in with Apple" text yet.

Maybe we did only do it inside our apps, but there is a default translation for "Sign in with Apple" for all languages Apple support, so developers should use that.

Maybe the blocker was that it would require a certain localization library as a dependency, which would make it a no-go, but maybe also Flutter change in the meantime and we could offer this now.

What I would love then would be to have a builder instead, where the developer can pass a Widget that gets the "default text".

But this is all beyond the scope of this one, and we can start putting this in first.

Thanks @IndigoSoftwares21.

@tp
Copy link
Collaborator

tp commented Mar 20, 2025

Is there any useful test we should duplicate for this functionality? Maybe just a simple rendering one (no need to for a golden test), that passes a widget like textWidget: Text('adsf') and then asserts on that being used?

@tp
Copy link
Collaborator

tp commented Mar 25, 2025

Couldn't add test to the original branch here, so did so in https://github.com/aboutyou/dart_packages/pull/464/files

Still not sure if we would want this, or if we should rather encourage developers to use their own "base button" of their choosing and drop in a AppleLogoPainter next to the text.

Because while it works fine in some standard cases, I could easily image some fonts needing more parameters (like padding) to make it fit in size and alignment with the logo (which is implicitly sized based on the height). Well, at least now developers could do a bit of that themselves with the Widget, which might be an improvement having just a smaller inteface via a TextStyle parameter…

@Dzivo
Copy link

Dzivo commented Apr 7, 2025

This will help us come closer to our designed button

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.

3 participants