feat: added a method to assign an id to the Section component#30
feat: added a method to assign an id to the Section component#30kenmistry wants to merge 1 commit into
Section component#30Conversation
|
@kenmistry is attempting to deploy a commit to the Web3templates Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds support for assigning stable HTML id attributes to section header areas so they can be linked to / scrolled to directly within the landing page.
Changes:
- Introduces a
sectionIdprop onSectionTitleand wires it toContainer. - Extends
Containerto optionally render adivid. - Enables global smooth scrolling via
scroll-behavior: smooth.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/components/SectionTitle.tsx |
Adds sectionId prop and passes it down to Container. |
src/components/Container.tsx |
Adds an optional idName prop and applies it as the rendered div’s id. |
src/app/page.tsx |
Assigns concrete ids (section-one … section-four) to each SectionTitle instance. |
src/app/globals.css |
Enables smooth scrolling globally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const SectionTitle = (props: Readonly<SectionTitleProps>) => { | ||
| return ( | ||
| <Container | ||
| idName={`${props.sectionId}`} |
There was a problem hiding this comment.
idName={${props.sectionId}} stringifies undefined into the literal "undefined". Because Container treats any non-empty string as truthy, this results in <div id="undefined"> when sectionId isn’t provided. Pass props.sectionId through directly (no template literal) and/or only set the prop when defined.
| idName={`${props.sectionId}`} | |
| idName={props.sectionId} |
| interface ContainerProps { | ||
| children: React.ReactNode; | ||
| idName?: string; | ||
| className?: string; |
There was a problem hiding this comment.
The new Container prop name idName is non-standard for a component that maps directly to a <div> attribute, and it makes call sites harder to read. Consider renaming this prop to id (or accepting React.HTMLAttributes<HTMLDivElement> / ...rest and spreading onto the <div>) to align with common React patterns and avoid a bespoke API surface.
| } | ||
|
|
||
| html { | ||
| scroll-behavior: smooth; |
There was a problem hiding this comment.
Enabling scroll-behavior: smooth globally can cause motion-sickness and violates user preference for reduced motion. Add a @media (prefers-reduced-motion: reduce) override (e.g., revert to auto) so smooth scrolling is disabled for those users.
| scroll-behavior: smooth; | |
| scroll-behavior: smooth; | |
| } | |
| @media (prefers-reduced-motion: reduce) { | |
| html { | |
| scroll-behavior: auto; | |
| } |
| interface SectionTitleProps { | ||
| sectionId?: string; | ||
| preTitle?: string; |
There was a problem hiding this comment.
PR title/description mention adding an id to a Section component, but the change introduces sectionId on SectionTitle and an idName prop on Container. Please align the PR metadata with the actual components being changed (or rename/refactor to match the intended Section abstraction) to avoid confusion for maintainers.
Hi, I am proposing a method to assign an
idattribute to theSectioncomponent so as to facilitate ease of locating via a unique identifier and of navigating between differentSectioncomponents.Not sure if this is needed at this time, given that the repo has not seen any updates for over a year. Happy to remove this PR if the repo is no longer maintained.