feat(android): introduce new prop fullScreen to adjust dialog screen size #378
Conversation
|
|
| super.onCreate(savedInstanceState) | ||
| authenticationAttempt = arguments?.getParcelable(AUTHENTICATION_ATTEMPT_KEY)!! | ||
| setStyle(STYLE_NORMAL, R.style.sign_in_with_apple_button_DialogTheme) | ||
| setStyle(STYLE_NORMAL, R.style.sign_in_with_apple_button_DialogTheme); |
There was a problem hiding this comment.
This becomes redundant if fullScreen is false. It could go in the else block for the condition below.
There was a problem hiding this comment.
FYI: when fullScreen is false, we're not rewriting the style over it, instead we only set/update the windowIsFloating style value to true
Please lmk if I am missing something. Thanks
| fun newInstance(authenticationAttempt: SignInWithAppleService.AuthenticationAttempt): SignInWebViewDialogFragment { | ||
| val fragment = SignInWebViewDialogFragment() | ||
| fun newInstance(authenticationAttempt: SignInWithAppleService.AuthenticationAttempt, fullScreen: Boolean = true): SignInWebViewDialogFragment { | ||
| val fragment = SignInWebViewDialogFragment(fullScreen) |
There was a problem hiding this comment.
Maybe this is not true for React Native but I would assume we needed to pass fullScreen via the bundle, for any apps that utilise rotation.
|
Hey there 👋 - apologies for how stale this repo had gotten. It was still working fine but the stale-ness made developing against it very difficult If you rebase this to current main you will hopefully see a much improved experience
and since I just implemented the new "Trusted Publish" changes npmjs.org just started enforcing Nov 1, once any changes here are complete, we can actually publish them 😄 Cheers |
|
@mikehardy No worries, thanks for taking a look at this! |
NJ-2020
changed the title
NJ-2020
changed the title
|
PR is ready, tested locally + lint check also passing ✅ cc: @mikehardy |
|
For IOS, I have build issues |
There was a problem hiding this comment.
This looks just about good to go - and I'm so pleased I did the CI work this week because I believe you have compile errors, but at same time I don't see them in CI so I'm going to say...the PR is probably okay - you must be having a local issue. You didn't touch iOS stuff anyway
The only thing that gives me pause is the direction of the boolean. Specifically:
Full screen mode
When true, the dialog fits within the app window only.
When false, it covers the entire screen including the status bar.
Defaults to true
🤔 it seems to me that if "fullScreen" is true, I should expect it will cover the entire screen. But...it appears this toggle works the opposite way?
As a developer reading this documentation, that surprises me.
Can we have it so that fullScreen=true means it covers the entire screen, and fullScreen=false means it does the app window only?
Either way, please let me say I really appreciate you (and it seems Expensify?) contributing PRs and such - it really helps 🙌
|
🤦 Ah damn, my bad! I accidentally wrote the boolean flag backwards. It should be: Sorry about that – fixed now! ☑️
Thank you – glad to help out! |
There was a problem hiding this comment.
okay, the docs look like they match up much better with a naive expectation :-)
last thing I can see - maybe I'm missing something but I still see two instances of "full screen will be false by default" in the PR - which is contrary to the docs, I proposed concrete suggestions to flip those default booleans in case my understanding is correct - so you can just hit the accept suggestions button and move on
either way, excited to merge this and release it - looks like it's just about ready
thanks again
android/src/main/java/com/RNAppleAuthentication/SignInWithAppleConfiguration.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/RNAppleAuthentication/SignInWithAppleConfiguration.kt
Outdated
Show resolved
Hide resolved
mikehardy
left a comment
mikehardy
left a comment
There was a problem hiding this comment.
excellent! Code looks good, I'll shepherd this through CI + merge + release, thanks!
|
v2.5.0 appears to be live now, thanks again for submitting this please let me know if the publish behaves in some unexpected way - I just fully automated it to handle the new npmjs.com token/security changes, so the yaml for that action has only been used twice... |
|
Thank you! I really appreciate you reviewing and merging this PR. |
4 participants
Fixes #377 by passing
fullScreen={false}Result
Screen.Recording.2025-11-06.at.09.50.58.mov