OBLS-932 Display customer delivery address on picking screens - #438
OBLS-932 Display customer delivery address on picking screens#438olewandowski1 wants to merge 1 commit into
Conversation
| address?: DestinationAddress | null; | ||
| }; | ||
|
|
||
| function compact(parts: Array<string | null | undefined>) { |
There was a problem hiding this comment.
As with useState, no technical requirement behind it and there is no "team convention", so we just use both declarations.
| import { ProductDetails } from '../../components/ProductDetails'; | ||
| import { HYPHEN } from '../../constants'; | ||
| import { DestinationAddress } from '../../types/picking'; | ||
| import styles from './customerDetailsStyles'; |
There was a problem hiding this comment.
Did you guys have a ticket for fixing the imports? I mean using aliases here
There was a problem hiding this comment.
Nope, as far as I'm aware. We talked about it several times, but no action items were created.
| } | ||
|
|
||
| export function CustomerDetails({ name, address }: CustomerDetailsProps) { | ||
| const [visible, setVisible] = React.useState(false); |
There was a problem hiding this comment.
Is there a reason for using useState called on a React object rather than importing the useState directly?
There was a problem hiding this comment.
No functional reason here. Just used it like that because the picking files follow this pattern.
| {label}: <Text style={[styles.chipText, styles.fontBold]}>{value ?? HYPHEN}</Text> | ||
| </Text> | ||
| </Chip> | ||
| function Item({ icon, label, value, secondaryValue, onPress, accessibilityLabel }: ProductDetailsItem) { |
There was a problem hiding this comment.
I don't know what the purpose of that component is, but I would expect Item to be able to receive a child component and render it, so that the content can 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.
I like the idea, and I agree making it composable is the right path. Nevertheless, this ProductDetails.Item is 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.
| <View style={styles.dialog}> | ||
| <View style={styles.header}> | ||
| <View style={styles.headerText}> | ||
| <Text style={styles.eyebrow}>CUSTOMER</Text> |
There was a problem hiding this comment.
I would use here destinationLocationType from currentTask not to limit ourselves that it will always be Customer. I would use { destinationLocationType || 'Customer' }
| <> | ||
| <ProductDetails.Item | ||
| icon="map-marker" | ||
| label="Customer" |
There was a problem hiding this comment.
I would use here destinationLocationType from currentTask not to limit ourselves that it will always be Customer. I would use { destinationLocationType || 'Customer' }
| <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} |
There was a problem hiding this comment.
I'll ask Emilia if the country is really necessary
3 participants
https://openboxes.atlassian.net/browse/OBLS-932
Summary
destinationAddressresponse