Use Deep Links for Copying Multiplayer Join Code, Update Host Lobby Appearance #9153
Conversation
…m the join code's appearance.
…layer/thsparks/rework_host_lobby
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
We should not show a space in the code:
Typing a space in the middle will be seen as an invalid join code. |
I updated the join code parsing to account for that. I think it's much easier to read with the space, personally... |
| {inviteString} | ||
| {inviteStringSegments[0]} | ||
| { | ||
| <a |
There was a problem hiding this comment.
Use Link component from react-common, rather than a.
multiplayer/src/util/index.ts
Outdated
| @@ -80,7 +80,7 @@ export function cleanupJoinCode( | |||
| joinCode: string | undefined | |||
| ): string | undefined { | |||
| if (!joinCode) return undefined; | |||
| joinCode = joinCode.trim(); | |||
| joinCode = joinCode.replaceAll(" ", ""); | |||
There was a problem hiding this comment.
Oh I see, this will remove the space. Maybe it's fine.
There was a problem hiding this comment.
Should we maybe just move line 85 up and delete this / line 84? That should handle this just fine as it's already clearing out all non alphanumeric already (which would fix if there's a weird nbsp or something)
| }; | ||
| const joinCode = state.gameState?.joinCode; | ||
| if (!joinCode) { | ||
| return <Loading />; |
There was a problem hiding this comment.
It was just something so it wasn't empty, but this case shouldn't ever really happen. It can be null instead if you think that's better.
There was a problem hiding this comment.
null is better, but not worth holding this PR
| }; | ||
| // To get a link in the middle of the invite string, we actually preserve the {0} and insert the link manually as an html element later. | ||
| const inviteString = lf("Go to {0} and enter code", "{0}"); | ||
| const inviteStringSegments = inviteString.split("{0}"); |
| return ( | ||
| <div className="tw-flex tw-flex-col tw-gap-1 tw-items-center tw-justify-between tw-bg-white tw-py-[3rem] tw-px-[7rem] tw-shadow-lg tw-rounded-lg"> | ||
| <div className="tw-mt-3 tw-text-lg tw-text-center tw-text-neutral-700"> | ||
| {inviteString} | ||
| {inviteStringSegments[0]} |
There was a problem hiding this comment.
Assuming the number of elements in inviteStringSegments feels brittle to me. And assuming the link is in the middle segment isn't a safe assumption. I think you will need to iterate over the segments, and replace the one matching "{0}" with the link.
There was a problem hiding this comment.
Hmm, currently we split on {0} so there's no actual segment containing it. The only scenario where there wouldn't be 2 segments is if it was at the beginning or the end, so would it be better if we simply checked startsWith and endsWith {0} and reacted accordingly?
There was a problem hiding this comment.
I guess if {0} ended up in multiple places that'd also mess things up but I don't expect that would happen as a result of localization.
There was a problem hiding this comment.
split will leave an empty string at beginning / end if the the string starts with / ends with it so that would still be fine, e.g.
I guess if the translation doubled up the link it would be an issue, but that would be pretty weird / we do this in a few other locations already and haven't seen that before?
There was a problem hiding this comment.
I think I would split on whitespace and find the {0}. You could optimize by rejoining the contiguous word sequences before and after it. Do this in a useMemo.
There was a problem hiding this comment.
But there a ways of solving it.
There was a problem hiding this comment.
What's the benefit? Feels like unnecessary complexity to me.
There was a problem hiding this comment.
Leaving this as-is for now b/c time crunch, but we can follow up later to understand concerns and address as needed.
This updates the host lobby appearance to match the latest designs more closely, and it will copy a deep link rather than the code itself when the copy button is pressed (on both the host lobby and the game page).
Also sneaking in a fix to make the sim use the full screen width when on a smaller screen.