[Feature] Create Link Component #162
Conversation
TimRoe
force-pushed
the
feature/118-roettger-CreateLinkComponent
branch
from
610158e to
c1488d4
Compare
| type nullTypeSpecifics = { | ||
| calendarData?: never | ||
| locationData?: never | ||
| /** Optional onPress override logic */ | ||
| onPress?: () => void | ||
| phoneNumber?: never | ||
| textNumber?: never | ||
| TTYnumber?: never | ||
| url?: never | ||
| } | ||
|
|
||
| type calendar = Omit<nullTypeSpecifics, 'calendarData'> & { | ||
| type: 'calendar' | ||
| calendarData: CalendarData | ||
| } | ||
|
|
||
| type call = Omit<nullTypeSpecifics, 'phoneNumber'> & { | ||
| type: 'call' | ||
| phoneNumber: string | ||
| } | ||
|
|
||
| type callTTY = Omit<nullTypeSpecifics, 'TTYnumber'> & { | ||
| type: 'call TTY' | ||
| TTYnumber: string | ||
| } | ||
|
|
||
| type custom = Omit<nullTypeSpecifics, 'onPress'> & { | ||
| type: 'custom' | ||
| /** Required onPress override logic */ | ||
| onPress: () => void | ||
| } | ||
|
|
||
| type directions = Omit<nullTypeSpecifics, 'locationData'> & { | ||
| type: 'directions' | ||
| locationData: LocationData | ||
| } |
There was a problem hiding this comment.
This convenience type is a little confusing but maybe because I haven't used Omit and never much. Is the thinking behind this because they all share the onPress? If so, wondering if this could be done by creating a type that has just the onPress and extending or intersecting it with each type. Might be a little clearer that way unless I'm missing something.
There was a problem hiding this comment.
The crux of the nullTypeSpecifics type is that TS gets confused if you don't include all props in all types--so, with it, basically the props calendarData, TTYnumber, etc. technically exist in the directions type so TS doesn't give weird errors about "so and so prop doesn't exist on directions." Then being set to "never" makes it so setting those props for a type they shouldn't be on will give a TS error. It's a bit annoying, but it was what was needed to keep the type prop flat when leveraging the component which I think is a worthwhile tradeoff even if it's a bit ugly on our side.
onPress was mainly lumped in with the never's because the custom type didn't work correctly when I had it in LinkProps. It seemed to partially register as the documentation became "Required onPress override logicOptional onPress override logic" (no space and all) for the custom type, but it remained optional instead of being required like it should.
So I think it'd be fine if we had an onPress type and then it became:
type directions = onPress & Omit<nullTypeSpecifics, 'locationData'> & { ... }
and then custom just wouldn't have the onPress &.
I'm kind of ambivalent about that being better than lumping all the type specifics together, even if onPress is an odd one out that the "null" case for it is being optional instead of required. The typing is pretty ugly any way you slice it on our side, but works well for our consumers to be able to fill in the necessary data as simple as possible with TS guardrails preventing doing it wrong.
As you will be picking up some Link-related tickets, maybe we leave it for this PR and you can play with it on your ticket to see if you find a way that you think is a step forward? I'm definitely open to there maybe being a better way, but TS gets very finicky about it.
There was a problem hiding this comment.
Split to a separate comment since it's just extra info not directly relevant:
My prior implementation had type as a required prop in LinkProps and then had the linkTypes within that, which made calling it look like:
<Link text='blah' type={{type: 'calendar', calendarData: calendarData}} />
which was super obnoxious and auto formed poorly because TS in no way indicated that type was an object so you wouldn't get any help autofilling until you put in the 2nd level of { }'s. This also entailed duplicating onPress so technically they could send it both flat and with custom inside the object. The TS as implemented allows it to nicely be flat like:
<Link text='blah' type='calendar' calendarData={calendarData} />
and works seamlessly for all types just like that.
| export const Phone: Story = { | ||
| args: { | ||
| text: 'Call number', | ||
| onPress: undefined, // Storybook sends a truthy function shell otherwise |
There was a problem hiding this comment.
You might be aware but this works when running on phone but I get an unhandled promise rejection when running in iOS sim. I know simulators don't support calls but I wonder if there's a way to suppress this warning.
There was a problem hiding this comment.
Haven't run on a simulator in ages since finding I could use my iOS device over Wi-Fi. RIP my Ad Hoc Android device being too old for Wi-Fi connection w/ Android Studio.
Does that pop up just generally running or only when pressing the link? If it's only when pressing it, feels obscure and I'm not sure how we'd handle better. The only thought coming to my mind for "handling" it would be making a dead tap, but that in some ways is worse as at least the warning indicates your onPress is working. Did flagship handle simulator specific stuff like phone links in a better way?
Description of Change
basevariant icons to not be a severe contrast violationdirectionstype will prompt user interaction before leavingOSfunctionsutilities setTesting Packages
Screenshots/Video
Android Storybook:
Screen_Recording_20240207-173402_Expo.Go.mp4
Android Flagship:
Screen_Recording_20240207-171145_VA.mp4
iOS Flagship:
RPReplay_Final1707339213.MP4
Testing
Draft tested around in all 3 Storybook versions as well as iOS/Android in the app with alpha build replacing a handful of links of various varieties.
PR Checklist
Code reviewer validation:
changeloglabel applied if it's to be included in the changelogPublish
While changes warrant a new version per the versioning guidelines, the component's state is rough and it is deemed wise to await subsequent tickets to publish a version officially releasing it.