-
Notifications
You must be signed in to change notification settings - Fork 42
[C-1799] Add slug support for collection deep-links #2680
base: main
Are you sure you want to change the base?
Conversation
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.
looks great!! thanks so much for doing this. i had a few questions we can discuss and then hopefully we can test everything soon
const handleFetchCollection = useCallback(() => { | ||
dispatch(fetchCollection(id)) | ||
}, [dispatch, id]) | ||
dispatch(fetchCollection(id, slug && decodeURIComponent(slug))) |
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.
why do we need to decode the slug here?
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.
Good question, so for urls that have special characters, they are typically encoded, so decoding here to get the right special characters back
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.
gotcha ty!
} = params ?? {} | ||
|
||
const id = useMemo(() => { | ||
if (collectionName) { |
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.
curious if you think we should keep this around for that edge case you mentioned earlier where playlists in local storage might not have permalinks yet?
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.
Yeah exactly what I was thinking as well, unless it's the case that the actions/sagas we are running take the permalink and extract handle id as a backup? Guessing that's not the case and I should send all three along in the fetch action?
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.
yeah that is not the case, so we'd have to send all 3 along in fetch 😬 yea i think we'd want to pass along in fetch? also realizing this is the screen used for albums too probably, and albums use the original routing and expect the name to contain id and name so we still need this for sure
Description
Updates mobile deep links to support playlist slug format
Dragons
Need to confirm if we also need to decostruct the "slug" to handle + id and pass all three along to fetch request.