Skip to content

add link to jump to session header #225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryoppippi merged 6 commits into from
Nov 8, 2025
Merged

add link to jump to session header #225

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8322e61
wip: add link to jump to session header
gamoutatsumi Nov 7, 2025
d3f6aee
use speaker_name and title as session header id
gamoutatsumi Nov 8, 2025
98a5180
set scroll-padding-top
gamoutatsumi Nov 8, 2025
d006c4f
use instead session_id
gamoutatsumi Nov 8, 2025
3cd51d6
fix css format issue
gamoutatsumi Nov 8, 2025
3eee4f6
fix prettier issue
gamoutatsumi Nov 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion 2025/src/components/Sessions/Session.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ const { speaker_name, title, abstract, self_introduction, image_url } =
</div>
<div class="flex-1 text-center md:text-left">
<div class="mb-3">
<h3 class="mb-2 text-2xl font-bold text-gray-900 md:text-3xl">
<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">
Copy link
Member

@ryoppippi ryoppippi Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

[nits]変数化してもいいかも？

@gamoutatsumi

Copy link
Member

@ryoppippi ryoppippi Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

copilotが言うように、session_idを使う方が簡素な気がするが、どっちがいいのだろう
一意なものならなんでもいい気もするが

Copy link
Member

@ryoppippi ryoppippi Nov 8, 2025

Choose a reason for hiding this comment

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

session_idは一意っぽいな。そっちのが良さそうかも
encodeは多分要らない？はず？

Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
<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">

Copilot uses AI. Check for mistakes.
{speaker_name}
</a>
</h3>
</div>
<div class="mb-4">
Expand Down
4 changes: 4 additions & 0 deletions 2025/src/styles/global.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ body {
font-family: Arial, Helvetica, sans-serif;
}

html {
scroll-padding-top: calc(var(--spacing) * 16)
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

Missing semicolon at the end of the CSS rule. This could cause issues with CSS parsing.

Suggested change
scroll-padding-top: calc(var(--spacing) * 16)
scroll-padding-top: calc(var(--spacing) * 16);

Copilot uses AI. Check for mistakes.
Copy link
Member

@ryoppippi ryoppippi Nov 8, 2025

Choose a reason for hiding this comment

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

@gamoutatsumi これも多分必要かも？

Copy link
Contributor Author

@gamoutatsumi gamoutatsumi Nov 8, 2025

Choose a reason for hiding this comment

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

修正済み

}

@layer utilities {
.text-balance {
text-wrap: balance;
Expand Down
Loading