Convert GiantTrackTile to typescript #3698
Conversation
Oh yep, I can try to add comments to the important bits. There aren't really any big logic changes. |
There was a problem hiding this comment.
@dharit-tan Added some comments about the major things that changed in this file
| actionGroupLabel: 'collection actions' | ||
| } | ||
|
|
||
| export type GiantTrackTileProps = { |
There was a problem hiding this comment.
These were all converted from GiantTrackTile.propTypes. However, that type just used things like object and string for some props which have more explicit types (i.e. Remix, PremiumConditions, CoverArtSizes). Pretty much anything in here that isn't a number/string/boolean is now more strongly typed.
| trackId, | ||
| trackTitle, | ||
| userId | ||
| }: GiantTrackTileProps) => { |
There was a problem hiding this comment.
This was originally the render function, and didn't destructure as many props because the helper functions defined on the class would read them directly from this.props. Now since we are defining the helpers inline here, they are read from closure and we need to pull more items off of props.
| FeatureFlags.PODCAST_CONTROL_UPDATES_ENABLED_FALLBACK | ||
| ) | ||
|
|
||
| const renderCardTitle = (className: string) => { |
There was a problem hiding this comment.
These were all converted from class methods. They all had their direct destructuring of this.props removed in favor of reading them from closure.
| /> | ||
| <div className={styles.infoSection}> | ||
| <div className={styles.infoSectionHeader}> | ||
| {renderCardTitle(cn(fadeIn))} |
There was a problem hiding this comment.
Had to add a cn() here because the helper function expects a simple string classname and fadeIn is an object with conditional classnames meant to be passed to the classnames function.
| <span> | ||
| {/* prop types for overflow menu don't work correctly | ||
| so we need to cast here */} | ||
| <Menu {...(overflowMenu as any)}> |
There was a problem hiding this comment.
Didn't want to go down a rabbit hole of trying to fix the props definition for Menu... 😢
| </div> | ||
| </div> | ||
|
|
||
| {isPremium && premiumConditions ? ( |
There was a problem hiding this comment.
Added a check here on premium conditions to avoid rendering this section altogether. Technically I think the child component is safe if it isn't passed. But in TrackTile we follow the pattern of not rendering the section if there are no premium conditions.
| <Linkify | ||
| options={{ | ||
| attributes: { | ||
| onClick: (event: React.MouseEvent<HTMLAnchorElement>) => { |
There was a problem hiding this comment.
This handler needed a more specific type.
| } | ||
|
|
||
| return <p className={styles.progressText}>{messages.unplayed}</p> |
There was a problem hiding this comment.
This might seem weird. TS was complaining that this component couldn't be used in JSX because it might return undefined. Lo and behold, it might!
The way the conditionals were structured, there's a rare chance (trackPosition.status being neither of the values we check against) we would return nothing from this function (though it shouldn't be likely), which would be an error in React.
|
Preview this change https://demo.audius.co/convert-giant-track-tile |
Description
Converts GiantTrackTile to typescript / function component pattern to make it easier to work with for upcoming gated content changes.
Nullableprops for a few cases where the values they received fromGiantTrackTilemight actually be null. The underlying code is already null-safe, the props just didn't reflect that.anycast for the overflow menu props, as TS is currently unable to correctly determine the track variant and it was becoming a hassle to try and refactor it so the checker would be happy. I don't love this, but since this component was already working and I didn't change how any of the values are passed/computed it seems like a safe cast for now.onExternalLinkClickedhandler. I'm not sure why we do this, but that handler passes the entire event up the component stack, as opposed to the internal version which just passes a url 😅. Again, goal was to convert with minimal functional changes.Dragons
There might be very subtle behaviors that stop working due to converting from class to functional component, or due to some of the minor changes I made.
How Has This Been Tested?
Tested on web, compared a variety of track states to the same version on staging to make sure now large regressions or inconsistencies popped up
How will this change be monitored?
Will test as part of full QA pass for this feature branch
Feature Flags
N/A