fix(native-app): Multiple license fixes #16744
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several modifications across various files, primarily focusing on enhancing error handling and user feedback related to wallet functionality and document rendering. New localized error messages are added for wallet pass operations, while improvements are made to the rendering logic in document detail and wallet pass screens. Additionally, updates to styling and conditional rendering logic are included to improve user experience and responsiveness of components. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
thoreyjona
force-pushed
the
fix/license-fixes
branch
from
f18fea1 to
1c2ea6d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16744 +/- ##
=======================================
Coverage 36.54% 36.54%
=======================================
Files 6890 6890
Lines 143640 143640
Branches 40926 40926
=======================================
Hits 52488 52488
Misses 91152 91152
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
apps/native/app/src/ui/lib/field/field.tsx (2)
Line range hint
26-31: Consider making the margin size configurable.While the current implementation works, consider making the margin size configurable through props for better component reusability across different contexts.
-const Value = styled.Text<{ size?: 'large' | 'small' }>` - margin-right: ${({ theme }) => theme.spacing[2]}px; +const Value = styled.Text<{ size?: 'large' | 'small'; marginSize?: keyof Theme['spacing'] }>` + margin-right: ${({ theme, marginSize = 2 }) => theme.spacing[marginSize]}px;
Line range hint
31-58: Enhance type safety for value prop.The component could benefit from stronger typing for the
valueprop to prevent potential runtime issues.interface FieldProps { label?: string | null - value?: string | null + value?: string | number | null loading?: boolean compact?: boolean size?: 'large' | 'small' style?: ViewStyle | null }apps/native/app/src/ui/lib/field/field-card.tsx (1)
154-156: Consider simplifying the conditional logic.The current condition is complex and contains redundant checks. Consider this simpler alternative that maintains the same behavior:
- {(props.title !== null && - !(props.title === undefined && props.code === undefined)) || - (props.title !== undefined && props.code) ? ( + {(props.title != null || props.code != null) ? (This simplified version:
- Uses
!= nullto check for bothnullandundefined- Maintains the requirement that at least one of
titleorcodemust be defined- Preserves the existing behavior for all license types
- Is more readable and maintainable
apps/native/app/src/screens/vehicles/vehicles-detail.tsx (1)
Line range hint
23-27: Enhance type safety with explicit type annotationsWhile the TypeScript implementation is generally good, we could improve type safety by:
- Adding explicit return type for the component
- Adding type annotations for destructured query results
Consider applying these changes:
-export const VehicleDetailScreen: NavigationFunctionComponent<{ +export const VehicleDetailScreen: NavigationFunctionComponent<{ title?: string id: string -}> = ({ componentId, title, id }) => { +}> = ({ componentId, title, id }): JSX.Element => {And for query results:
const { mainInfo, basicInfo, registrationInfo, inspectionInfo, technicalInfo, - } = data?.vehiclesDetail || {} + } = data?.vehiclesDetail || {} as { + mainInfo: typeof data.vehiclesDetail.mainInfo + basicInfo: typeof data.vehiclesDetail.basicInfo + registrationInfo: typeof data.vehiclesDetail.registrationInfo + inspectionInfo: typeof data.vehiclesDetail.inspectionInfo + technicalInfo: typeof data.vehiclesDetail.technicalInfo + }apps/native/app/src/ui/lib/card/license-card.tsx (2)
312-314: Consider extracting barcode dimensions logic.While the calculations are correct, consider extracting them into a utility function for better reusability and testability.
+const calculateBarcodeDimensions = (screenWidth: number, spacing: Theme['spacing']) => { + const width = screenWidth - spacing[4] * 2 - spacing.smallGutter * 2 + const height = width / 3 + return { width, height } +} export function LicenseCard({...}) { - const barcodeWidth = screenWidth - theme.spacing[4] * 2 - theme.spacing.smallGutter * 2 - const barcodeHeight = barcodeWidth / 3 + const { width: barcodeWidth, height: barcodeHeight } = calculateBarcodeDimensions(screenWidth, theme.spacing)
400-412: LGTM: Clean implementation of offline message.The offline message implementation is well-structured and consistent with the design system. However, consider extracting the background opacity value to a constant or theme variable for better maintainability.
+const OFFLINE_MESSAGE_BG_OPACITY = 0.4 {showBarcodeOfflineMessage && !showBarcodeView && ( <BarcodeWrapper minHeight={barcodeHeight}> <BarcodeContainer - style={{ backgroundColor: 'rgba(255,255,255,0.4)' }} + style={{ backgroundColor: `rgba(255,255,255,${OFFLINE_MESSAGE_BG_OPACITY})` }} >apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (2)
177-181: Consider simplifying the spacing calculation logicThe complex conditional for
informationTopSpacingcould be more readable by breaking it down into smaller, named conditions.- const informationTopSpacing = - (allowLicenseBarcode && ((loading && !data?.barcode) || data?.barcode)) || - (!isConnected && isBarcodeEnabled) - ? barcodeHeight + LICENSE_CARD_ROW_GAP - : 0 + const shouldShowBarcode = allowLicenseBarcode && ((loading && !data?.barcode) || data?.barcode) + const isOfflineWithBarcodeEnabled = !isConnected && isBarcodeEnabled + const informationTopSpacing = + (shouldShowBarcode || isOfflineWithBarcodeEnabled) + ? barcodeHeight + LICENSE_CARD_ROW_GAP + : 0
249-264: Consider consolidating duplicate alert logicThe error alert logic is duplicated between the Android and iOS code paths. Consider extracting this into a reusable function.
+ const showErrorAlert = (canAddPass: boolean, licenseType?: string) => { + Alert.alert( + intl.formatMessage({ id: 'walletPass.errorTitle' }), + !canAddPass + ? intl.formatMessage({ id: 'walletPass.errorCannotAddPasses' }) + : licenseType === 'DriversLicense' + ? intl.formatMessage({ id: 'walletPass.errorNotPossibleToAddDriverLicense' }) + : intl.formatMessage({ id: 'walletPass.errorAddingOrFetching' }), + licenseType === 'DriversLicense' + ? [ + { + text: intl.formatMessage({ id: 'walletPass.moreInfo' }), + onPress: () => Linking.openURL('https://island.is/okuskirteini'), + }, + { + text: intl.formatMessage({ id: 'walletPass.alertClose' }), + style: 'cancel', + }, + ] + : [] + ) + }Also applies to: 292-310
apps/native/app/src/screens/document-detail/document-detail.tsx (1)
105-112: LGTM! Consider adding aspect-ratio preservation.The CSS changes correctly prevent SVG and image overflow by setting max-width to 100%. While this is a good fix, consider adding
height: autoto preserve aspect ratio and prevent distortion.svg { max-width: 100%; display: block; + height: auto; } img { max-width: 100%; display: block; + height: auto; }apps/native/app/src/messages/en.ts (1)
262-264: Consider making error messages more specific and actionable.The error messages could be more helpful by:
- Specifying where to install Smartwallet from (e.g., App Store/Play Store)
- Providing more context about why the fetch/add operation failed
- 'walletPass.errorCannotAddPasses': - 'You cannot add passes. Please make sure you have Smartwallet installed on your device.', - 'walletPass.errorAddingOrFetching': 'Failed to fetch or add pass.', + 'walletPass.errorCannotAddPasses': + 'You cannot add passes. Please install Smartwallet from the App Store/Play Store to continue.', + 'walletPass.errorAddingOrFetching': 'Failed to fetch or add pass. Please check your connection and try again.',apps/native/app/src/messages/is.ts (1)
398-405: LGTM! Consider adding periods for message consistency.The new wallet pass error messages are well-structured and provide clear user guidance. They align with the PR objectives and follow the established patterns.
For consistency with other error messages in the file, consider adding periods at the end of each message:
'walletPass.errorCannotAddPasses': - 'Þú getur ekki bætt við skírteini. Vinsamlegast vertu viss um að þú sért með Smartwallet appið sett upp á tækinu.', + 'Þú getur ekki bætt við skírteini. Vinsamlegast vertu viss um að þú sért með Smartwallet appið sett upp á tækinu..', 'walletPass.errorAddingOrFetching': - 'Tókst ekki að sækja eða bæta við skírteini.', + 'Tókst ekki að sækja eða bæta við skírteini..', 'walletPass.errorNotPossibleOnThisDevice': - 'Þú getur ekki bætt við skírteinum á þetta tæki.', + 'Þú getur ekki bætt við skírteinum á þetta tæki..', 'walletPass.errorNotConnectedNoBarcode': - 'Ekki er hægt að skanna skírteini nema að tækið sé nettengt.', + 'Ekki er hægt að skanna skírteini nema að tækið sé nettengt..',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/native/app/src/messages/en.ts(1 hunks)apps/native/app/src/messages/is.ts(1 hunks)apps/native/app/src/screens/document-detail/document-detail.tsx(1 hunks)apps/native/app/src/screens/vehicles/vehicles-detail.tsx(1 hunks)apps/native/app/src/screens/wallet-pass/wallet-pass.tsx(10 hunks)apps/native/app/src/ui/lib/card/license-card.tsx(6 hunks)apps/native/app/src/ui/lib/field/field-card.tsx(1 hunks)apps/native/app/src/ui/lib/field/field.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/native/app/src/messages/en.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/is.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/document-detail/document-detail.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/vehicles/vehicles-detail.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/card/license-card.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/field/field-card.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/field/field.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (10)
apps/native/app/src/ui/lib/field/field.tsx (1)
26-26: LGTM: Margin addition aligns with PR objectives.
The added margin-right improves the spacing between fields, which directly addresses the PR objective of adding margins between firearm fields.
apps/native/app/src/ui/lib/field/field-card.tsx (1)
154-156: LGTM! The changes successfully prevent empty headers.
The updated condition correctly handles the case where both title and code are undefined, preventing the rendering of empty headers for firearms while maintaining existing behavior for other license types.
apps/native/app/src/screens/vehicles/vehicles-detail.tsx (1)
17-20: LGTM: Bottom tab configuration properly implemented
The navigation options correctly implement the requirement to hide the bottom tab bar on the vehicle details screen. The drawBehind: true setting ensures proper layout handling.
apps/native/app/src/ui/lib/card/license-card.tsx (3)
36-37: LGTM: Clean implementation of Typography and screen dimensions.
The new imports and styled component follow the project's patterns and maintain consistency with the design system.
Also applies to: 116-120
275-275: LGTM: Well-defined prop interface.
The new optional prop follows TypeScript best practices and boolean naming conventions.
293-293: LGTM: Clean props destructuring.
The new prop is correctly destructured following the existing pattern.
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (3)
39-39: LGTM: Clean offline state management implementation
The addition of offline state management through useOfflineStore is well-integrated and follows React best practices.
Also applies to: 150-150
414-414: LGTM: Improved layout handling
The updates to layout handling, including:
- Offline message display
- Dynamic content inset calculations
- Proper margin handling with SafeAreaView
These changes effectively address the PR objectives for improving the display of wallet passes and offline states.
Also applies to: 418-424, 427-435
183-188: 🛠️ Refactor suggestion
Review the necessity of key-based re-render
While the current implementation works, forcing re-renders using a key is a temporary solution. Consider refactoring to use proper state management or component lifecycle methods.
apps/native/app/src/messages/en.ts (1)
265-268: LGTM! Clear and informative error messages.
The error messages for device compatibility and offline mode are clear, concise, and provide good user feedback. They align well with the PR objectives, particularly the addition of an offline placeholder for barcodes.
|
@thoreyjona maybe add max width on text so the second line won't only have one word |
* fix: don't show bottom tabs on vehicle details screen * feat: fix padding issues for licenses and add errors to translation files * feat: add offline placeholder for barcodes * fix: fire arm license fixes * fix: make sure images and svgs in html documents are not super big * fix: remove contentContainer style * fix: better condition for field-card * fix: make sure there is not one word in line 2 for offline message --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Multiple license fixes:
Additional fixes:
Screenshots / Gifs
Removed most of the information since these are production screenshots.
Image fix for html documents:
Before vs after:
Fixing of margin of firearm fields:
Before and after:
Offline placeholder for barcodes:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Styling