Skip to content

Add share button#22

Merged
v-ji merged 9 commits intomainfrom
21-share-button
Sep 9, 2025
Merged

Add share button#22
v-ji merged 9 commits intomainfrom
21-share-button

Conversation

@nel-hei
Copy link
Contributor

@nel-hei nel-hei commented Mar 17, 2025

I have added a Share button that appears with and below the ‘Back to top’ button when scrolling to the main section of the website. Clicking on it displays a message that a share link has been copied.

Closes #21.

@nel-hei nel-hei added the enhancement New feature or request label Mar 17, 2025
@nel-hei nel-hei requested a review from v-ji March 17, 2025 14:39
@nel-hei nel-hei changed the title Add share button (#21) Add share button Mar 17, 2025
@nel-hei nel-hei marked this pull request as draft March 17, 2025 14:44
Copy link
Member

@v-ji v-ji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! In terms of UX, let’s introduce an intermediary step between the share button and the link copying. This should be a floating dialogue next to the buttons which contains a small explanatory text (that the user can copy and share the link to this exact play). Below, the resulting URL should be visible (but truncated) along with a “copy” button.

With this increased scope, the component can be moved to its own file (in src/lib/components).

When initialising state from a shared URL, please trigger a scroll to the <main> element (already bound as mainElement). We may add onto this with a small notification or similar.

@nel-hei nel-hei requested a review from v-ji March 31, 2025 14:32
@nel-hei
Copy link
Contributor Author

nel-hei commented Mar 31, 2025

I've just noticed that if you manually change a single digit of the sequence by scrolling horizontally, the share link doesn't update. Still need to fix this.

Copy link
Member

@v-ji v-ji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Nele, thank you for your changes! Things are looking good and functional so far, just a few changes and comments:

I have removed the ?share=true URL parameter, as it is not needed to keep track of whether the user has arrived via a shared URL – any time sequence state is initialised from the URL hash, we know it’s from a shared URL.

Also, i fixed a race condition in the feature that scrolls to the main element when arriving from a shared URL: We first have to wait for the components to mount and the element to become available.

It would be great if you could address the individual comments before we can move into the specifics and also involve Mark for design/UX :)

@v-ji v-ji marked this pull request as ready for review September 9, 2025 12:26
@v-ji v-ji merged commit 9a463c0 into main Sep 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add share button

2 participants