adding feature of anchor links #960
Conversation
|
@arkid15r done |
Summary by CodeRabbit
WalkthroughThis PR refines test selectors to accurately target contributor sections by specifying a class ( Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/components/ItemCardList.tsx (1)
52-52: Text formatting inconsistency in fallback message.The fallback message has inconsistent spacing with extra spaces before and after commas and periods.
- <p>Currently , Nothing to Display . </p> + <p>Currently, Nothing to Display.</p>frontend/src/pages/Home.tsx (1)
169-182: Well-implemented anchor link for New Projects section.The implementation follows the same pattern as other sections, providing consistent navigation.
The ID
new-Projectshas inconsistent capitalization. Consider maintaining all-lowercase or kebab-case for IDs:- <div id="new-Projects" className="relative scroll-mt-20"> + <div id="new-projects" className="relative scroll-mt-20">Also update the corresponding href attribute on line 174.
frontend/src/components/CardDetailsPage.tsx (3)
128-164: Consider enhancing anchor links accessibility for keyboard users.The implementation for Languages and Topics sections looks good, but the hover-only visibility of the anchor links might present accessibility issues for keyboard or touch device users.
Consider making the link icon visible when the element receives keyboard focus as well:
- className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" + className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100 group-focus-within:opacity-100"
195-218: Consider adding aria-label to anchor links for better screen reader support.The anchor links implementation for Recent Releases is good, but lacks explicit screen reader support.
Add an aria-label to improve accessibility:
<a href="#recent-releases" className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" + aria-label="Link to Recent Releases section" >
41-240: Consider creating a reusable component for section headers with anchor links.There is significant repetition in the anchor link pattern across multiple sections. This could be refactored into a reusable component to reduce duplication and make future maintenance easier.
Example implementation:
// SectionHeader component const SectionHeader = ({ id, title }: { id: string; title: string }) => ( <div id={id} className="relative scroll-mt-20"> <div className="group relative flex items-center"> {title} <a href={`#${id}`} className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" aria-label={`Link to ${title} section`} > <FontAwesomeIcon icon={faLink} className="h-7 w-5" /> </a> </div> </div> ); // Usage <SecondaryCard title={<SectionHeader id="summary" title="Summary" />}> <p>{summary}</p> </SecondaryCard>🧰 Tools
🪛 Biome (1.9.4)
[error] 76-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(7 hunks)frontend/src/components/ItemCardList.tsx(3 hunks)frontend/src/components/SecondaryCard.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/components/ToggleableList.tsx(1 hunks)frontend/src/index.css(1 hunks)frontend/src/pages/Home.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (24)
frontend/src/components/SecondaryCard.tsx (1)
8-8: Good type enhancement for title propChanging the type from
stringtoReact.ReactNodeimproves flexibility by allowing the title to accept complex JSX elements including anchor links, which aligns with the PR objective of adding anchor links to major blocks.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
101-101: Improved test selector precisionThe change from
.closest('div')to.closest('div.mb-8')provides a more specific DOM selector, ensuring the test correctly identifies the contributors section with its new anchor link structure.frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
102-102: Improved test selector precisionThe change from
.closest('div')to.closest('div.mb-8')provides a more specific DOM selector, ensuring the test correctly identifies the contributors section with its new anchor link structure.frontend/src/components/ToggleableList.tsx (2)
5-5: Added React import for JSX supportAdding the React import is necessary when using React.ReactNode type and JSX syntax in the component.
13-13: Good type enhancement for label propChanging the type from
stringtoReact.ReactNodeimproves flexibility by allowing the label to accept complex JSX elements including anchor links, which aligns with the PR objective of adding anchor links to major blocks.frontend/src/index.css (2)
297-299: Good addition of smooth scrolling behavior!The
scroll-behavior: smoothproperty provides a better user experience by adding a smooth animation when users navigate to anchor links, rather than jumping abruptly to the target location.
301-303: Well implemented utility class for color inheritance.This
.inherit-colorclass allows elements (like the anchor links) to take on the color of their parent elements, maintaining visual consistency while still being interactive elements.frontend/src/components/ToggleContributors.tsx (2)
2-2: Clean addition of the link icon import.The FontAwesome link icon is appropriately imported alongside the existing icons.
32-42: Well-structured anchor link implementation.This implementation:
- Creates a proper anchor point with ID
top-contributors- Uses
scroll-mt-20to offset the scroll position and prevent the heading from being hidden under any fixed headers- Implements a clean hover effect where the link icon only appears when hovering over the section heading
- Uses the
inherit-colorclass to maintain style consistencyThe grouping of elements and relative positioning is also well done.
frontend/src/components/ItemCardList.tsx (2)
2-2: Appropriate addition of React import.The React import is necessary for using the React.ReactNode type.
11-11: Good type update to support JSX in title.Changing the title prop type from string to React.ReactNode enables passing JSX elements, which is necessary for implementing the anchor links with icons.
frontend/src/pages/Home.tsx (5)
8-8: Clean addition of the link icon import.The FontAwesome link icon is correctly imported alongside the existing icons.
132-144: Well-implemented anchor link for New Chapters section.The structure provides a good user experience with:
- Proper ID-based anchor
- Scroll margin to prevent header overlap
- Hover effect for the link icon
- Consistent styling using the
inherit-colorclass
211-221: Well-structured anchor link for Chapters Nearby section.The implementation maintains consistency with the other sections while correctly wrapping the h2 element.
230-243: Properly implemented anchor link for Recent Issues section.The anchor structure is well implemented with appropriate hover effects and styling.
254-267: Properly implemented anchor link for Recent Releases section.The anchor is correctly structured with consistent styling and behavior matching the other sections.
frontend/src/components/CardDetailsPage.tsx (8)
1-1: FontAwesome import updated to include faLink icon.Good addition of the
faLinkicon which is used for the anchor links throughout the component.
41-57: Anchor link implementation for Summary section adds navigation improvement.The addition of anchor links provides a nice way for users to link directly to specific sections. The implementation using hover-based visibility is clean and unobtrusive.
60-85: Anchor link implementation for Details section follows consistent pattern.The same anchor link pattern is consistently applied to the Details section.
🧰 Tools
🪛 Biome (1.9.4)
[error] 76-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
87-107: Statistics section anchor link looks good.The anchor implementation for the Statistics section maintains the consistent pattern seen throughout the component.
171-194: Recent Issues section anchor link implementation is consistent.The anchor link implementation follows the same pattern as other sections, maintaining consistency throughout the UI.
222-240: Repositories section anchor link implemented consistently.The anchor link pattern is maintained for the Repositories section, ensuring a consistent user experience.
48-48:❓ Verification inconclusive
Verify that the 'inherit-color' class is defined in your global CSS.
The
inherit-colorclass is used for the anchor links, but it's not defined in this file. Ensure it's properly defined in your global CSS to avoid styling issues.Also applies to: 67-67, 94-94, 137-137, 155-155, 178-178, 202-202, 229-229
🏁 Script executed:
#!/bin/bash # Check if the inherit-color class is defined in the CSS files echo "Searching for inherit-color class definition..." rg "\.inherit-color" -t css -t scss echo "Searching for inherit-color in tailwind config..." rg "inheritColor|inherit-color" -t js -t ts tailwindLength of output: 384
Action Required: Verify Global CSS Definition for "inherit-color"
The automated search did not provide conclusive evidence that the "inherit-color" class is defined in your global CSS. It appears either the definition is missing or is located in a file/type not covered by the search. Please manually verify that the global stylesheet includes a definition for
.inherit-color(e.g.,.inherit-color { color: inherit; }) to ensure the anchor link styling works as intended.
43-43:❓ Verification inconclusive
Ensure smooth scrolling behavior is enabled in your global CSS.
The
scroll-mt-20class suggests you're implementing smooth scrolling with an offset. Make sure you have defined smooth scrolling behavior in your global CSS.Also applies to: 62-62, 89-89, 132-132, 150-150, 173-173, 197-197, 224-224
🏁 Script executed:
#!/bin/bash # Check if smooth scrolling is defined in the CSS files echo "Searching for scroll-behavior definition..." rg "scroll-behavior:\s*smooth" -t css -t scss echo "Checking for smooth scroll in global HTML/CSS..." rg "html\s*{[^}]*scroll-behavior:\s*smooth" -t css -t scss echo "Checking for scroll-mt-20 class in tailwind config..." rg "scrollMargin|scroll-mt" -t js -t ts tailwindLength of output: 687
Ensure global CSS implements smooth scrolling behavior
The file at
frontend/src/components/CardDetailsPage.tsx(line 43) applies thescroll-mt-20class to create an offset for smooth scrolling. However, our automated search did not conclusively locate a global CSS rule—such ashtml { scroll-behavior: smooth; }—that enables smooth scrolling. This might be due to configuration differences or search limitations.Please manually verify that:
- Your global CSS (e.g., a file like
styles/global.css) contains a rule similar to:html { scroll-behavior: smooth; }- Your Tailwind configuration is correctly set up to support custom scroll margin classes. Similar usages were found on lines 62, 89, 132, 150, 173, 197, and 224, so consistency across the codebase is important.
If the smooth scrolling behavior is not defined globally, add the necessary CSS rule to ensure consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
frontend/src/components/TitleWithIcon.tsx (5)
4-8: Props interface looks good but consider making title more flexible.The interface is well-defined, but you might want to consider using
React.ReactNodeinstead ofstringfor thetitleprop to allow for more complex title content including nested elements.interface TitleWithIconProps { href: string icon: IconProp - title: string + title: React.ReactNode }
1-38: Consider adding CSS transition for smoother interactions.The current implementation only has a transition for opacity. Consider adding transitions for other properties to make the interaction smoother.
<a href={href} - className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" + className="inherit-color ml-2 opacity-0 transform scale-95 transition-all duration-200 group-hover:opacity-100 group-hover:scale-100" onClick={handleClick} aria-label={`Link to ${typeof title === 'string' ? title : 'this section'}`} title={`Link to ${typeof title === 'string' ? title : 'this section'}`} >
4-38: Add JSDoc comments for better code documentation.Adding JSDoc comments would improve documentation and provide better IDE hints for users of this component.
+/** + * Props for the TitleWithIcon component + */ interface TitleWithIconProps { href: string icon: IconProp title: React.ReactNode } +/** + * A component that renders a title with an anchor link icon that appears on hover. + * Clicking the icon smoothly scrolls to the corresponding section. + * + * @param props - The component props + * @param props.href - The href attribute for the anchor link (should start with #) + * @param props.icon - The FontAwesome icon to display + * @param props.title - The title content to display + * @returns A React component + */ const TitleWithIcon: React.FC<TitleWithIconProps> = ({ href, icon, title }) => { // Component implementation... }
22-23: Improve screen reader compatibility.The current implementation uses
items-topwhich is not a standard Tailwind class. Additionally, the class structure could be improved for better accessibility and maintainability.- <div id={id} className="relative scroll-mt-20"> - <div className="group relative flex items-top"> + <div id={id} className="relative scroll-mt-20 group"> + <div className="relative flex items-start">
21-34: Enhance user experience with focus states.Consider adding focus states for better keyboard navigation support, ensuring the component is fully accessible.
return ( <div id={id} className="relative scroll-mt-20"> <div className="group relative flex items-top"> <h2 className="mb-4 text-2xl font-semibold">{title}</h2> <a href={href} - className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" + className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100 focus:opacity-100 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500" onClick={handleClick} aria-label={`Link to ${typeof title === 'string' ? title : 'this section'}`} > <FontAwesomeIcon icon={icon} className="h-7 w-5" /> </a> </div> </div> )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/CardDetailsPage.tsx(8 hunks)frontend/src/components/TitleWithIcon.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/pages/Home.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/ToggleContributors.tsx
- frontend/src/pages/Home.tsx
- frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (1)
frontend/src/components/TitleWithIcon.tsx (1)
1-2:❓ Verification inconclusive
Verify FontAwesome dependency inclusion.
Ensure that the FontAwesome packages are correctly listed in your package.json and that you're importing only the icons you need to minimize bundle size.
🏁 Script executed:
#!/bin/bash # Check FontAwesome packages in package.json echo "Checking FontAwesome packages in package.json..." grep -E "@fortawesome" package.json || echo "FontAwesome packages not found in package.json" # Check if there's a central icon import file echo "Checking for central icon imports..." find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "import.*from.*@fortawesome" | sort # Check if faLink is imported somewhere echo "Checking for faLink imports..." find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "faLink" | sortLength of output: 1912
Action: Validate your FontAwesome dependency setup.
In
frontend/src/components/TitleWithIcon.tsx(lines 1–2), you're importing modules from FontAwesome. However, our check for apackage.jsonat the repository root did not find any FontAwesome-related dependencies. If your project uses a separatepackage.json(for example, within thefrontend/directory), please confirm that packages such as@fortawesome/react-fontawesomeand@fortawesome/fontawesome-svg-coreare listed there. Also, ensure you import only the icons you truly need to reduce bundle size.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/pages/Home.tsx (1)
131-234:⚠️ Potential issueMissing ID attributes for anchor links.
While you've added anchor links with hrefs (e.g., "#new-chapters", "#recent-issues"), I don't see corresponding ID attributes on the target HTML elements. For the anchor links to work correctly, you need to add matching ID attributes to the elements they should navigate to.
Add ID attributes to each section that corresponds to the anchor href. For example:
- <div className="grid gap-4 md:grid-cols-2"> + <div id="new-chapters" className="grid gap-4 md:grid-cols-2">Apply this pattern to all sections that need anchor links:
- Add
id="new-chapters"to the div at line 131- Add
id="new-projects"to the div at line 157- Add
id="chapters-nearby"to the div at line 187- Add
id="recent-issues"to the div at line 195- Add
id="recent-releases"to the div at line 207
🧹 Nitpick comments (5)
frontend/src/pages/Home.tsx (5)
158-158: Fix capitalization inconsistency in the anchor ID.There's an inconsistency in the anchor ID format. All other anchor IDs use lowercase, but this one has a capital 'P' in "Projects".
- title={<TitleWithIcon href="#new-Projects" icon={faLink} title="New Projects" />} + title={<TitleWithIcon href="#new-projects" icon={faLink} title="New Projects" />}
112-253: Consider implementing smooth scrolling for anchor links.Adding smooth scrolling behavior would enhance the user experience when navigating between sections using the anchor links. This can be done using CSS.
You can add the following CSS to your global styles:
html { scroll-behavior: smooth; }Or add it to your styled component/CSS-in-JS solution if you're using one. This will ensure smooth scrolling when users click on anchor links.
188-193: Implement a more semantic section structure for "OWASP Chapters Nearby".For better accessibility and semantic HTML structure, consider wrapping this section in a
<section>element or using appropriate ARIA roles.- <div className="mb-20"> + <section id="chapters-nearby" className="mb-20"> <TitleWithIcon href="#chapters-nearby" icon={faLink} title="OWASP Chapters Nearby" /> <ChapterMap geoLocData={geoLocData} style={{ height: '400px', width: '100%', zIndex: '0' }} /> - </div> + </section>
194-223: Implement a consistent approach to container elements.For "Recent Issues" and "Recent Releases", consider adding a wrapper with an ID to each section, rather than relying on the ItemCardList component to serve as the anchor target. This would create a more consistent approach with the other sections.
- <div className="grid gap-4 md:grid-cols-2"> + <div className="grid gap-4 md:grid-cols-2"> + <div id="recent-issues"> <ItemCardList title={<TitleWithIcon href="#recent-issues" icon={faLink} title="Recent Issues" />} data={data.recentIssues} renderDetails={(item) => ( /* ... existing code ... */ )} /> + </div> + <div id="recent-releases"> <ItemCardList title={<TitleWithIcon href="#recent-releases" icon={faLink} title="Recent Releases" />} data={data.recentReleases} renderDetails={(item) => ( /* ... existing code ... */ )} /> + </div> </div>
132-223: Consider making the anchor link icon more accessible.For better accessibility, consider adding more context to the anchor links, such as screen reader text that explains the purpose of the links.
In your
TitleWithIconcomponent, you could enhance accessibility with:<a href={href} className="anchor-link" aria-label={`Link to ${title} section`} > <FontAwesomeIcon icon={icon} /> <span className="sr-only">Link to {title} section</span> </a>This ensures screen readers can properly convey the purpose of these anchor links to users with visual impairments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/CardDetailsPage.tsx(8 hunks)frontend/src/components/TitleWithIcon.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/pages/Home.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/ToggleContributors.tsx
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/components/TitleWithIcon.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (3)
frontend/src/pages/Home.tsx (3)
8-8: The imports forfaLinkandTitleWithIconlook good.These imports properly support the anchor link functionality being added to the page.
Also applies to: 27-27
132-134: Anchor link implementation for "New Chapters" looks correct.The title has been properly updated to use the
TitleWithIconcomponent with appropriate parameters.
132-159: Address the scrolling issue mentioned in previous review.In previous reviews, it was noted that clicking anchor links in the two-column layout (specifically the second card) causes the page to scroll to the top instead of staying in place. This issue was acknowledged but doesn't appear to be fixed in this update.
To properly test this issue, we need to check if clicking the anchor link for "New Projects" (which is in the second column) causes undesired scrolling behavior.
#!/bin/bash # Look for any scroll prevention measures or anchor-related JS in the codebase echo "Checking for scroll prevention measures in TitleWithIcon component..." fd "TitleWithIcon" -t f --exec cat {} \; | grep -A 10 -B 10 "onClick\|preventDefault\|scroll" echo "Checking for any smooth scroll implementations..." rg "scroll-behavior|scrollIntoView" --type=css rg "scrollIntoView|preventDefault" --type=ts --type=tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/AnchorTitle.tsx (3)
5-8: Consider making the title prop more flexible.The
titleprop is currently typed as a string, which limits flexibility. Consider allowing React nodes (ReactNode type) to support more complex title content like formatted text or nested elements.interface AnchorTitleProps { href: string - title: string + title: React.ReactNode }
11-11: Add more robust ID generation.The current ID generation simply removes '#' characters, but IDs should be unique and valid HTML IDs. Consider a more comprehensive approach to handle special characters and ensure valid ID generation.
- const id = href.replace('#', '') + const id = href.replace('#', '').replace(/[^a-zA-Z0-9-_:]/g, '-').toLowerCase()
22-33: Enhance accessibility of the anchor link component.While the component functions well visually, it could benefit from additional accessibility attributes for screen readers and keyboard navigation.
<div id={id} className="relative scroll-mt-20"> <div className="items-top group relative flex"> <h2 className="mb-4 text-2xl font-semibold">{title}</h2> <a href={href} className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" onClick={handleClick} + aria-label={`Link to ${typeof title === 'string' ? title : 'this section'}`} + title={`Link to ${typeof title === 'string' ? title : 'this section'}`} > <FontAwesomeIcon icon={faLink} className="h-7 w-5" /> </a> </div> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/AnchorTitle.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(7 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/pages/Home.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/ToggleContributors.tsx
- frontend/src/pages/Home.tsx
- frontend/src/components/CardDetailsPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (3)
frontend/src/components/AnchorTitle.tsx (3)
13-19: Good implementation of smooth scrolling functionality.The click handler properly prevents default behavior and uses the smooth scrolling API. The null check before accessing the element is also a good practice.
22-22: Good use of scroll-mt-20 for scroll positioning.The
scroll-mt-20class ensures the element isn't hidden under fixed headers when scrolled to. This is a thoughtful implementation detail for a better user experience.
27-27:✅ Verification successful
Verify the color inheritance styling.
The class
inherit-coloris used for the anchor element, but it's not a standard Tailwind class. Ensure this class is properly defined in your CSS to prevent styling issues.
🏁 Script executed:
#!/bin/bash # Check if the inherit-color class is defined in the project's CSS files echo "Searching for inherit-color class definition..." rg -t css "inherit-color" --no-headingLength of output: 183
Action: Confirm CSS Definition for "inherit-color"
The verification shows that the
inherit-colorclass is defined infrontend/src/index.css, so there are no styling issues with the anchor element. No further changes are needed.
|
@kasya so for that i think we have to improve n refactor the code snippets and make a seperate file which have the duplicated logic as a single function and importing that to re - use it to solve that duplication problem
that'll i have analysed Thank you : ) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/pages/Home.tsx (1)
175-175: Fix case inconsistency in the href attribute.There's a case inconsistency in the "New Projects" href attribute that might cause navigation issues. Anchor link URLs are case-sensitive.
-<SecondaryCard title={<AnchorTitle href="#new-Projects" title="New Projects" />}> +<SecondaryCard title={<AnchorTitle href="#new-projects" title="New Projects" />}>Also applies to: 204-204
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/e2e/pages/Home.spec.ts(4 hunks)frontend/src/components/CardDetailsPage.tsx(6 hunks)frontend/src/pages/Home.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/e2e/pages/Home.spec.ts
- frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (5)
23-23: Good addition of the AnchorTitle component import.This import enables the anchor link functionality throughout the page, supporting the PR objective of adding navigable sections.
137-137: LGTM! The AnchorTitle implementation looks good.The title is now wrapped with AnchorTitle, providing a clickable anchor and smooth scrolling to the "Upcoming Events" section.
238-238: Good standalone usage of AnchorTitle.Using AnchorTitle directly here (without wrapping it in SecondaryCard) is appropriate for this section heading.
248-248: Consistent implementation of anchor links in ItemCardList.The AnchorTitle component is now properly integrated with ItemCardList components, maintaining a consistent navigation pattern throughout the page.
Also applies to: 264-264
204-204:❓ Verification inconclusive
Verify scroll behavior for two-column layouts.
Based on previous review comments, there were issues with anchor links in two-column layouts scrolling to the top of the page instead of staying in place. Please ensure this behavior has been fixed with the current implementation.
🏁 Script executed:
#!/bin/bash # Check for any position-related CSS that might affect scrolling behavior cd frontend grep -r "position:" --include="*.css" --include="*.scss" src/components/AnchorTitle*Length of output: 101
Anchor Scroll Behavior Verification
The current changes in the
AnchorTitlecomponent did not reveal any CSS modifications related to positioning when checked via our grep search. However, because previous issues with anchor links in two-column layouts were observed (resulting in the view scrolling to the top), please manually verify in the browser that the fix works as intended. Confirm that clicking an anchor link in a two-column layout maintains the proper scroll position rather than jumping to the top.
|
@kasya |
There was a problem hiding this comment.
@shining-bluemoon-11 Thanks for updating this! The scroll on click works as epected, but I noticed one issue with scroll on getting to the url with an anchor tag, like http://localhost:3000/#recent-issues for example. I left a separate comment about this further down.
Also, I noticed that when we click anchor tags - there's no more changes made to the URL (we do not update it to have a relative path to the spot on the page).
I believe I saw it working like that at some point.. but not anymore.
Can we update this? The whole point of having these anchor links is to be able to share a link to a specific spot on the page and for that we need to know the url for it.
There was a problem hiding this comment.
The scroll works fine when you click on the page. However I noticed that even if I copy and paste the link to a specific spot, on navigating to that address the scroll still works incorrectly, scrolling more than needed.
Here's a video of scrolling on click and on using the link:
https://www.awesomescreenshot.com/video/37646610?key=ed1cc573c52082f8dce672f1d5036191
There was a problem hiding this comment.
@kasya , ok
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/components/AnchorTitle.tsx (5)
16-19: Abstract the scroll offset calculation.The current implementation uses magic numbers (-50) for scroll offset calculation. This makes the code less maintainable and harder to understand.
- const headingHeight = element.querySelector('h2')?.offsetHeight || 0 - const yOffset = -headingHeight - 50 - const y = element.getBoundingClientRect().top + window.pageYOffset + yOffset + // Define constants at the top of the component or in a separate file + const ADDITIONAL_OFFSET = 50 + const headingHeight = element.querySelector('h2')?.offsetHeight || 0 + const yOffset = -headingHeight - ADDITIONAL_OFFSET + const y = element.getBoundingClientRect().top + window.scrollY + yOffset
30-30: Use the same hash extraction logic in both places.For consistency, use the same method to extract the hash here as in the handlePopState function.
- const hash = window.location.hash.replace('#', '') + const hash = window.location.hash.substring(1)
38-38: Use substring instead of replace for consistency with the href validation.If you implement the suggested fix for the href validation on line 11, you should update this line to match that pattern.
- const hash = window.location.hash.replace('#', '') + const hash = window.location.hash.substring(1)
48-59: Adjust the icon vertical alignment as previously requested.The icon doesn't appear vertically centered compared to the text, as mentioned in the previous review.
<div id={id} className="relative"> - <div className="items-top group relative flex"> + <div className="group relative flex items-center"> <h2 className="mb-4 text-2xl font-semibold">{title}</h2> <a href={href} className="inherit-color ml-2 opacity-0 transition-opacity duration-200 group-hover:opacity-100" onClick={handleClick} > - <FontAwesomeIcon icon={faLink} className="custom-icon h-7 w-5" /> + <FontAwesomeIcon icon={faLink} className="custom-icon h-5 w-5 align-middle" /> </a> </div> </div>
5-8: Consider making the title prop more flexible.The title prop is currently typed as a string, but it might be useful to support React nodes for more complex titles.
interface AnchorTitleProps { href: string - title: string + title: React.ReactNode }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/__tests__/e2e/pages/Home.spec.ts(5 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(1 hunks)frontend/src/components/AnchorTitle.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(6 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/components/ToggleableList.tsx(2 hunks)frontend/src/index.css(1 hunks)frontend/src/pages/Home.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/components/ToggleContributors.tsx
- frontend/src/components/ToggleableList.tsx
- frontend/tests/e2e/pages/Home.spec.ts
- frontend/tests/unit/pages/RepositoryDetails.test.tsx
- frontend/tests/unit/pages/ProjectDetails.test.tsx
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/pages/Home.tsx
- frontend/src/index.css
🔇 Additional comments (1)
frontend/src/components/AnchorTitle.tsx (1)
11-11: Fix the href validation as previously suggested.The current implementation doesn't validate if
hrefstarts with '#'. If an invalid href is provided, it could lead to unexpected behavior.- const id = href.replace('#', '') + const id = href.startsWith('#') ? href.substring(1) : href
|
@kasya , i have make changes Thank you : ) |
|
@kate , I didn't noticed that frontend check , |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/About.test.tsx (1)
186-186: Consider using data-testid instead of relying on utility class selectorsThe change to
.closest('div.mb-8') as HTMLElementmakes the selector more specific, which likely aligns with the new anchor links structure. However, relying on utility classes likemb-8(margin-bottom) for test selectors can make tests brittle, as these classes might change for styling purposes without considering test impact.Consider adding a dedicated
data-testidattribute to the contributors section container for more reliable test selection:- .closest('div.mb-8') as HTMLElement + .closest('[data-testid="contributors-section"]') as HTMLElementThis would require adding the corresponding data-testid attribute to the component being tested, but would make the tests more resilient to styling changes.
Additionally, consider adding a null check instead of type assertion to handle potential null returns from
.closest():- .closest('div.mb-8') as HTMLElement + .closest('div.mb-8') + if (!contributorsSection) throw new Error('Contributors section not found');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/About.test.tsx(1 hunks)frontend/src/pages/Home.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Home.tsx
|
|
@shining-bluemoon-11 There are some conflicts now in 2 files. Could you update those and run tests locally to make sure it's all good? Would love to merge this in. |
|
@kasya |
RESOLVE #926
Tasks Completed:
VID20250228221656.mp4