-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
J=SLAP-2441 TEST=auto
cbbbb34
to
64844e2
Compare
Code Coverage / call_code_coverage / base-coverage Percy Snapshots / call_percy_snapshots / percy_snapshots |
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 noticed eslint in this repo is using slapshot version, should this be using '@yext/eslint-config
instead? If not, can you update the config to extends '@yext/slapshot/typescript-react'
instead of '@yext/slapshot/typescript
resolved in other commit that edited that line #2 (comment) |
}: AddressLineProps) { | ||
const addressLineEls: ReactElement[] = line.map((fieldName, i) => { | ||
const key = `${fieldName}-${i}`; | ||
const value = fieldName === ',' ? separator : address[fieldName]; |
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.
when would fieldName === ','
be true? little confused why fieldName would be a comma
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.
Commas are an additional "type" in AddressPart to be specified in the lines prop. So lines: [['line1', ',', 'line2']]
would look like this "123 Main Street, apt #2" while lines: [['line1', 'line2']]
would look like this "123 Main Street apt #2". The purpose of this is although extremely uncommon clients will have particular points about where the commas in an address should go and not go.
Would not be opposed to building the commas in and making customizing where they go as out of scope
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.
can you add slapshot linting github action to this repo as well?
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.
Looking at the Address
Component, I think there are a number of foundational issues. That's no knock on this PR! We're porting the Component over from PagesJS. The Component represents the format of an Address as a matrix. I can see why someone chose to do it that way, but it's not intuitive. It certainly won't feel intuitive to an external developer who wants to alter the format. Likewise, I don't think htmlProps
is especially intuitive either.
For now, I think we should remove lines
, seperator
, and htmlProps
from the Address
interface. I don't think they make for a good customization experience for the developer. Also, by removing these props, we're free to change the implementation of the Component. We can get away from the "format as matrix" paradigm. Implementation-wise, I think we'd also want to look for a 3rd party library to do the localization instead of a manually maintained mapping.
With this PR, we'll have an Address
Component that produces the canonical, localized form of an Address. We can then talk with Product about how much customization we should allow in this Component and how.
export function Address({ | ||
address | ||
}: AddressProps) { | ||
const renderedLines = localeAddressFormat(address.countryCode) |
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.
do you think we'll ever need other address from the Address type, or will we always only use the countryCode?
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.
it's only for countryCodes
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.
Maybe this component would just take in the countryCode as a prop instead of the whole Address? If we think long term it will only ever need countryCode
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.
It needs the other ones to print as the address. The function localeAddressFormat()
just happens to only need a countryCode
I removed but to specify these I like the For |
prettier will come later right? |
@@ -0,0 +1,407 @@ | |||
import { AddressFieldName } from './types'; | |||
|
|||
export function localeAddressFormat(locale: string): AddressFieldName[][] { |
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.
is this something we want to include in this library? it seems like a pretty specific opinion on how to format addresses?
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.
Every country has an official or culturally popular format for how an address is formatted, so this is useful for that. Consider a US vs Switzerland address. The US has many cities with the same name so state names are crucial. Switzerland geographically is much smaller and doesn't even have states/regions. Even if all the document.address data is provided, it is important that we present the information in a way end users are familiar with.
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.
@oshi97, I wouldn't say these are opinions exactly. They represent the canonical address format for a particular locale or country. In France (or Switzerland) addresses don't follow the US format. Going forward, I think we should find a third party library to do this for us. Manually maintaining this ourselves seems like we're making life unnecessarily difficult.
renders an address based on the format specified in
lines
. Falls back to official localized address formatJ=SLAP-2441
TEST=auto