add link to jump to session header #225
Conversation
gamoutatsumi
changed the title
ryoppippi
requested review from
Copilot,
mopp,
ryoppippi and
staticWagomU
and removed request for
mopp and
staticWagomU
| <h3 id={encodeURIComponent(`${speaker_name}-${title}`)} class="mb-2 text-2xl font-bold text-gray-900 md:text-3xl"> | ||
| <a href={`#${encodeURIComponent(`${speaker_name}-${title}`)}`} class="hover:underline"> |
There was a problem hiding this comment.
[nits]変数化してもいいかも？
There was a problem hiding this comment.
copilotが言うように、session_idを使う方が簡素な気がするが、どっちがいいのだろう
一意なものならなんでもいい気もするが
There was a problem hiding this comment.
session_idは一意っぽいな。そっちのが良さそうかも
encodeは多分要らない？はず？
There was a problem hiding this comment.
Pull Request Overview
This PR adds anchor link functionality to session cards, allowing users to link directly to specific sessions. The implementation includes scroll padding to account for fixed headers when navigating to anchored sections.
Key Changes:
- Added CSS scroll padding to ensure anchored content is properly visible below fixed headers
- Implemented clickable anchor links on session speaker names with hover effects
- Generated unique IDs for each session based on speaker name and title
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| 2025/src/styles/global.css | Added scroll-padding-top to the html element for proper anchor navigation |
| 2025/src/components/Sessions/Session.astro | Added ID and anchor link to speaker names for direct linking to sessions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2025/src/styles/global.css
Outdated
| } | ||
|
|
||
| html { | ||
| scroll-padding-top: calc(var(--spacing) * 16) |
There was a problem hiding this comment.
Missing semicolon at the end of the CSS rule. This could cause issues with CSS parsing.
| scroll-padding-top: calc(var(--spacing) * 16) | |
| scroll-padding-top: calc(var(--spacing) * 16); |
| <h3 id={encodeURIComponent(`${speaker_name}-${title}`)} class="mb-2 text-2xl font-bold text-gray-900 md:text-3xl"> | ||
| <a href={`#${encodeURIComponent(`${speaker_name}-${title}`)}`} class="hover:underline"> |
There was a problem hiding this comment.
Using encodeURIComponent for HTML id attributes is problematic. HTML IDs have specific character restrictions and encodeURIComponent produces characters like % that, while technically valid in HTML5, can cause issues with CSS selectors and some JavaScript methods. Additionally, combining speaker name and title could result in very long IDs. Consider using a simpler ID like the session_id prop that's already available, or create a slug function that replaces spaces with hyphens and removes special characters.
| <h3 id={encodeURIComponent(`${speaker_name}-${title}`)} class="mb-2 text-2xl font-bold text-gray-900 md:text-3xl"> | |
| <a href={`#${encodeURIComponent(`${speaker_name}-${title}`)}`} class="hover:underline"> | |
| <h3 id={Astro.props.session_id} class="mb-2 text-2xl font-bold text-gray-900 md:text-3xl"> | |
| <a href={`#${Astro.props.session_id}`} class="hover:underline"> |
2 participants
set speaker_name and session title as id of header in session card
and set scroll-padding-top to adjust anchor scroll event