feat(expo): Part 1 - add native logo max height theme option#9154
feat(expo): Part 1 - add native logo max height theme option#9154swolfand wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 87eef36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExpo AuthView gains an optional ChangesExpo logo height support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthView
participant NativeClerkAuthView
participant ClerkAuthViewModule
participant ClerkInlineAuthWrapperView
AuthView->>NativeClerkAuthView: Forward logoMaxHeight
NativeClerkAuthView->>ClerkAuthViewModule: Set native prop
ClerkAuthViewModule->>ClerkInlineAuthWrapperView: Pass platform configuration
ClerkInlineAuthWrapperView->>ClerkInlineAuthWrapperView: Apply max-height logo rendering
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
swolfand
changed the title
API Changes Report
Summary
@clerk/expoCurrent version: 3.7.4 Subpath
|
|
Is this approach better than a param on the AuthView? This feels very AuthView specific to be sticking in the theme |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/expo/src/__tests__/appPlugin.theme.test.js (1)
128-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the zero boundary.
Since validation explicitly allows non-negative values, add
logoMaxHeight: 0to a valid-input test to guard the lower boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/expo/src/__tests__/appPlugin.theme.test.js` around lines 128 - 135, Add logoMaxHeight: 0 to an existing valid-input test for validateThemeJson, ensuring the zero boundary is covered as an accepted non-negative value while preserving the current invalid-input cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/expo/src/__tests__/appPlugin.theme.test.js`:
- Around line 128-135: Add logoMaxHeight: 0 to an existing valid-input test for
validateThemeJson, ensuring the zero boundary is covered as an accepted
non-negative value while preserving the current invalid-input cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d09edc0e-d13c-4c95-bd97-2a4106077ecc
📒 Files selected for processing (5)
.changeset/expo-native-logo-max-height.mdpackages/expo/android/src/main/java/expo/modules/clerk/ClerkExpoModule.ktpackages/expo/app.plugin.jspackages/expo/ios/ClerkNativeBridge.swiftpackages/expo/src/__tests__/appPlugin.theme.test.js
|
@mikepitre good call, switched that |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/expo/src/native/AuthView.tsx`:
- Around line 40-45: Validate logoMaxHeight in AuthView before passing it to
either native implementation: reject or coerce values that are non-finite or
negative, while preserving valid values. Add coverage for non-finite and
negative inputs, confirming invalid values never cross the native bridge.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ae113a9-e40d-489b-825b-5c21a4881f57
📒 Files selected for processing (10)
.changeset/expo-native-logo-max-height.mdpackages/expo/.npmrcpackages/expo/android/src/main/java/expo/modules/clerk/ClerkAuthViewModule.ktpackages/expo/ios/ClerkAuthNativeView.swiftpackages/expo/ios/ClerkNativeBridge.swiftpackages/expo/src/native/AuthView.tsxpackages/expo/src/native/AuthView.types.tspackages/expo/src/native/__tests__/AuthView.test.tsxpackages/expo/src/specs/NativeClerkAuthView.android.tspackages/expo/src/specs/NativeClerkAuthView.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/expo-native-logo-max-height.md
| export function AuthView({ | ||
| mode = 'signInOrUp', | ||
| isDismissible = true, | ||
| logoMaxHeight, | ||
| onDismiss, | ||
| }: AuthViewProps): ReactElement { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby references
git ls-files 'packages/expo/src/native/AuthView.tsx' 'packages/expo/**' | sed -n '1,120p'
echo
echo "== AuthView.tsx =="
cat -n packages/expo/src/native/AuthView.tsx | sed -n '1,220p'
echo
echo "== Search for logoMaxHeight =="
rg -n "logoMaxHeight" packages/expo -S
echo
echo "== Search for AuthViewProps =="
rg -n "type AuthViewProps|interface AuthViewProps|AuthViewProps" packages/expo -SRepository: clerk/javascript
Length of output: 11533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AuthView props =="
cat -n packages/expo/src/native/AuthView.types.ts | sed -n '1,180p'
echo
echo "== iOS Auth view =="
cat -n packages/expo/ios/ClerkAuthNativeView.swift | sed -n '1,180p'
echo
echo "== Android Auth view =="
cat -n packages/expo/android/src/main/java/expo/modules/clerk/ClerkAuthViewModule.kt | sed -n '1,220p'
echo
echo "== Native spec =="
cat -n packages/expo/src/specs/NativeClerkAuthView.ts | sed -n '1,120p'
echo
cat -n packages/expo/src/specs/NativeClerkAuthView.android.ts | sed -n '1,120p'Repository: clerk/javascript
Length of output: 11312
Validate logoMaxHeight before forwarding it to native.
It’s passed through unchanged, and both native implementations use it directly. Reject or coerce non-finite or negative values before crossing the bridge, and add coverage for invalid inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/expo/src/native/AuthView.tsx` around lines 40 - 45, Validate
logoMaxHeight in AuthView before passing it to either native implementation:
reject or coerce values that are non-finite or negative, while preserving valid
values. Add coverage for non-finite and negative inputs, confirming invalid
values never cross the native bridge.
Source: Coding guidelines
| setNeedsHostedViewUpdate() | ||
| } | ||
|
|
||
| func setLogoMaxHeight(_ logoMaxHeight: Double?) { |
There was a problem hiding this comment.
Can this parameter directly take a CGFloat? instead of conversion after?
| @@ -0,0 +1 @@ | |||
|
|
|||
3 participants
Summary
logoMaxHeightprop to Expo's nativeAuthViewClerkDesign.logoMaxHeightfor thatAuthView.clerkAppIcon(maxHeight:)on iOSWhy
The native Clerk Android and iOS SDKs support configuring the managed application logo height, but Expo apps could not pass that value to
AuthView.Because the setting only affects authentication screens, it is exposed as a component prop rather than as part of the global native theme configuration:
Summary by CodeRabbit
New Features
logoMaxHeightsetting to Expo’s nativeAuthView.Tests