-
-
Notifications
You must be signed in to change notification settings - Fork 12
OBLS-932 Display customer delivery address on picking screens #438
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import React from 'react'; | ||
| import { Modal, TouchableOpacity, View } from 'react-native'; | ||
| import { Divider, Text } from 'react-native-paper'; | ||
| import Icon from 'react-native-vector-icons/MaterialCommunityIcons'; | ||
|
|
||
| import { ProductDetails } from '../../components/ProductDetails'; | ||
| import { HYPHEN } from '../../constants'; | ||
| import { DestinationAddress } from '../../types/picking'; | ||
| import styles from './customerDetailsStyles'; | ||
|
Comment on lines
+6
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you guys have a ticket for fixing the imports? I mean using aliases here
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, as far as I'm aware. We talked about it several times, but no action items were created. |
||
|
|
||
| type CustomerDetailsProps = { | ||
| name?: string; | ||
| address?: DestinationAddress | null; | ||
| }; | ||
|
|
||
| function compact(parts: Array<string | null | undefined>) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you use arrow functions?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As with |
||
| return parts.filter(Boolean).join(', '); | ||
| } | ||
|
|
||
| function AddressDetails({ address }: { address?: DestinationAddress | null }) { | ||
| const addressLine1 = address?.address || HYPHEN; | ||
| const locality = compact([address?.city, address?.stateOrProvince, address?.postalCode]); | ||
| const hasAddress = Boolean( | ||
| address?.address || address?.address2 || locality || address?.country || address?.description | ||
| ); | ||
|
|
||
| return ( | ||
| <> | ||
| <Text style={styles.addressLine}>{addressLine1}</Text> | ||
| {address?.address2 ? <Text style={styles.addressLine}>{address.address2}</Text> : null} | ||
| {locality ? <Text style={styles.addressLine}>{locality}</Text> : null} | ||
| {address?.country ? <Text style={styles.addressLine}>{address.country}</Text> : null} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll ask Emilia if the country is really necessary |
||
| {address?.description ? <Text style={styles.description}>{address.description}</Text> : null} | ||
| {!hasAddress ? ( | ||
| <Text style={styles.missingAddress}>No delivery address is available in customer master data.</Text> | ||
| ) : null} | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export function CustomerDetails({ name, address }: CustomerDetailsProps) { | ||
| const [visible, setVisible] = React.useState(false); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for using useState called on a React object rather than importing the useState directly?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No functional reason here. Just used it like that because the picking files follow this pattern. |
||
|
|
||
| return ( | ||
| <> | ||
| <ProductDetails.Item | ||
| icon="map-marker" | ||
| label="Customer" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use here |
||
| value={name || HYPHEN} | ||
| secondaryValue={address?.address} | ||
| accessibilityLabel={`View delivery address for ${name || 'customer'}`} | ||
| onPress={() => setVisible(true)} | ||
| /> | ||
|
|
||
| <Modal transparent visible={visible} animationType="fade" onRequestClose={() => setVisible(false)}> | ||
| <View style={styles.overlay}> | ||
| <View style={styles.dialog}> | ||
| <View style={styles.header}> | ||
| <View style={styles.headerText}> | ||
| <Text style={styles.eyebrow}>CUSTOMER</Text> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use here |
||
| <Text style={styles.title}>{name || HYPHEN}</Text> | ||
| </View> | ||
| <TouchableOpacity | ||
| style={styles.closeButton} | ||
| accessibilityRole="button" | ||
| accessibilityLabel="Close customer details" | ||
| onPress={() => setVisible(false)} | ||
| > | ||
| <Icon name="close" size={20} style={styles.closeIcon} /> | ||
| </TouchableOpacity> | ||
| </View> | ||
|
|
||
| <Divider /> | ||
|
|
||
| <View style={styles.addressSection}> | ||
| <Icon name="map-marker-outline" size={22} style={styles.addressIcon} /> | ||
| <View style={styles.addressText}> | ||
| <Text style={styles.eyebrow}>SHIP TO ADDRESS</Text> | ||
| <AddressDetails address={address} /> | ||
| </View> | ||
| </View> | ||
| </View> | ||
| </View> | ||
| </Modal> | ||
| </> | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the purpose of that component is, but I would expect
Itemto be able to receive a child component and render it, so that thecontentcan be a separate component and passed as a prop. But I may not be right in your case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, and I agree making it composable is the right path. Nevertheless, this
ProductDetails.Itemis used across multiple screens, so the refactor would broaden this ticket's scope. We can create a separate ticket for it and keep it outside OBLS-932.