fix: align 'Show more' button in Projects page-#1329 #1894
Conversation
Summary by CodeRabbit
WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
Hi @arkid15r 👋 I've submitted a PR for the issue I was assigned. Please take a moment to review it. If everything looks good and works as expected, feel free to merge it. Thank you! 🙂 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/ToggleableList.tsx (1)
52-52: Consider extracting button styles to improve maintainability.The button has an extensive inline className with multiple utility classes. For better maintainability and reusability, consider extracting these styles into a CSS class or a styled component constant.
+const toggleButtonStyles = "bg-blue-50 hover:bg-blue-100 dark:bg-blue-900/20 dark:hover:bg-blue-900/30 text-blue-600 hover:text-blue-700 dark:text-blue-400 dark:hover:text-blue-300 border border-blue-200 dark:border-blue-800 rounded-md px-4 py-2 font-medium transition-colors" <Button disableAnimation onPress={toggleShowAll} - className="bg-blue-50 hover:bg-blue-100 dark:bg-blue-900/20 dark:hover:bg-blue-900/30 text-blue-600 hover:text-blue-700 dark:text-blue-400 dark:hover:text-blue-300 border border-blue-200 dark:border-blue-800 rounded-md px-4 py-2 font-medium transition-colors" + className={toggleButtonStyles} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ToggleableList.tsx(1 hunks)frontend/src/components/TopContributorsList.tsx(1 hunks)
🔇 Additional comments (6)
frontend/src/components/ToggleableList.tsx (3)
47-64: LGTM: Effective alignment fix with comprehensive styling.The flexbox container with
justify-startproperly aligns the button to the left, addressing the original issue. The enhanced button styling with background colors, borders, and dark mode support creates a consistent and polished UI component.
47-64: Excellent alignment fix with comprehensive styling!The button alignment issue has been properly addressed by wrapping it in a flex container with
justify-start. The enhanced Tailwind CSS classes provide consistent styling with proper dark mode support, background colors, borders, and smooth transitions.
56-56: Good improvement to icon spacing.The icon margin adjustment from
ml-1toml-2 text-smimproves visual balance and consistency. Thetext-smclass ensures the icon size is appropriate for the button context.Also applies to: 60-60
frontend/src/components/TopContributorsList.tsx (3)
70-86: LGTM: Consistent alignment fix implementation.The changes mirror those in
ToggleableList.tsx, providing consistent button alignment and styling across both components. The flexbox container effectively addresses the alignment issue mentioned in the PR objectives.
70-86: Perfect alignment fix for the Top Contributors section!This directly addresses the issue mentioned in #1329. The button is now properly aligned within the card using the flex container with
justify-start. The styling is consistent with the ToggleableList component, ensuring a unified UI experience across the application.
74-74: Comprehensive button styling with excellent accessibility.The detailed Tailwind classes provide:
- Proper contrast ratios for both light and dark modes
- Smooth color transitions for better UX
- Clear visual hierarchy with background, border, and text colors
- Consistent padding and border radius
This styling approach enhances both usability and visual appeal.
|
I will submit u the PR with the requested changes shortly @arkid15r |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ToggleButton.tsx (1)
17-28: LGTM! Clean conditional rendering implementation.The conditional rendering logic is well-structured and the icon/text combinations are semantically correct. The wrapper div provides consistent alignment.
Consider wrapping with
React.memoif this component re-renders frequently:+import React from 'react'; + // ... other imports -const ToggleButton = ({ +const ToggleButton = React.memo(({ // ... props -}) => ( +}) => ( // ... JSX -); +));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/ToggleButton.tsx(1 hunks)frontend/src/components/ToggleableList.tsx(2 hunks)frontend/src/components/TopContributorsList.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/ToggleableList.tsx
- frontend/src/components/TopContributorsList.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/src/components/ToggleButton.tsx (1)
Learnt from: Rajgupta36
PR: #1717
File: frontend/tests/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
🔇 Additional comments (3)
frontend/src/components/ToggleButton.tsx (3)
1-3: LGTM!The imports are well-organized and all are being used in the component.
5-10: Add TypeScript interface for props.The component is missing TypeScript type definitions, which reduces type safety and IDE support.
Apply this diff to add proper TypeScript typing:
+interface ToggleButtonProps { + isExpanded: boolean; + onToggle: () => void; + showMoreText?: string; + showLessText?: string; +} + -const ToggleButton = ({ +const ToggleButton = ({ isExpanded, onToggle, showMoreText = "Show more", showLessText = "Show less" -}) => ( +}: ToggleButtonProps) => (⛔ Skipped due to learnings
Learnt from: kasya PR: OWASP/Nest#1680 File: frontend/src/components/SponsorCard.tsx:3-3 Timestamp: 2025-06-30T00:55:03.133Z Learning: In the OWASP/Nest project, interfaces or types are not created for component props that are used only once. Inline prop type definitions are preferred for single-use cases.Learnt from: ahmedxgouda PR: OWASP/Nest#1714 File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12 Timestamp: 2025-07-08T17:07:50.988Z Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.Learnt from: Rajgupta36 PR: OWASP/Nest#1717 File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0 Timestamp: 2025-07-12T17:12:25.807Z Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
12-26: Confirm HeroUI Button’s defaulttypeor add explicittype="button"I wasn’t able to locate the
@heroui/buttonsource innode_modules/to verify its defaulttypehandling. To prevent unintended form submissions, please:
Verify in your project’s
@heroui/buttonpackage (or its docs) whether<Button>defaults totype="button".If it does not set
type="button", update yourToggleButton.tsxusage accordingly:<Button
- type="button"
disableAnimation
onPress={onToggle}
className="…"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Could you add tests for the new component? I bumped this to 10 points.
Thanks for the update! @arkid15r 🙌 If it’s okay and you or someone else is able to take care of the tests for now, that would be really helpful. I’ll definitely try to learn this soon and contribute better next time 🙇♂️ Thank you for your understanding! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ShowMoreButton.tsx (1)
23-32: Conditional rendering logic is correct and accessible.The component properly toggles between expanded and collapsed states with appropriate icons and text. However, consider adding an
aria-expandedattribute for better accessibility.<Button type="button" disableAnimation onPress={onToggle} + aria-expanded={isExpanded} className="flex items-center bg-transparent text-blue-400" >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/ShowMoreButton.tsx(1 hunks)frontend/src/components/ToggleableList.tsx(2 hunks)frontend/src/components/TopContributorsList.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/TopContributorsList.tsx
- frontend/src/components/ToggleableList.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
🔇 Additional comments (4)
frontend/src/components/ShowMoreButton.tsx (4)
5-15: Props interface looks good with sensible defaults.The component props are well-defined with appropriate types and default values for the text labels. The optional props pattern allows for customization while providing sensible defaults.
17-22: Button styling addresses the alignment requirements.The button uses consistent styling with transparent background and blue text color. The
flex items-centerensures proper icon alignment within the button.
5-36: Consider the trade-off between reusability and YAGNI principle.Based on the retrieved learning, the maintainer previously preferred not creating separate components for limited use cases. However, since this component addresses a specific UI alignment issue and is used in exactly two places, the extraction seems justified for:
- Consistency: Ensures identical styling and behavior across both usage locations
- Maintainability: Centralizes the fix for the alignment issue
- Bug Prevention: Reduces the risk of inconsistent implementations
The component strikes a good balance between reusability and simplicity.
16-16: Alignment fix verified for ShowMoreButtonThe
mt-4 flex justify-startwrapper in frontend/src/components/ShowMoreButton.tsx correctly enforces left alignment in both usages:
- frontend/src/components/TopContributorsList.tsx
- frontend/src/components/ToggleableList.tsx
No further changes are required.
|
Hi @arkid15r, I've made the following improvements:
Let me know if this works or if you'd like further changes! |
|
Hi @arkid15r, |
|
Hi @arkid15r, I noticed that @coderabbitai referenced this PR in another one (#1908), and I'm not quite sure what's happening here. Could you please help me understand the context or explain what exactly is going on? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you understand what's happening! Let me check PR #1908 to see the context of why I referenced this PR. 🏁 Script executed: Length of output: 27536 @adithya-naik I can explain what happened! When I reviewed PR #1908 (which is about making API endpoints available and refactoring backend environment detection logic), I referenced a learning from our conversation in this PR (#1894). Here's what occurred: In this PR (#1894), you mentioned your preference not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering. I stored this as a "learning" in my knowledge base to remember your coding preferences for future reviews. When I reviewed PR #1908, this learning was relevant to some aspect of the code changes there, so it appeared in the "Learnings" section of my review comment. This is how I maintain context about maintainer preferences across different PRs to provide more personalized and consistent reviews. This is completely normal behavior - it helps me give better reviews by remembering project-specific preferences and patterns. The reference doesn't mean the PRs are directly related; it just means I applied knowledge from our previous interaction to inform my review of the other PR. You can see this learning referenced in the "🧠 Learnings" section of my comment on PR #1908, where it shows it was "Learnt from: adithya-naik, PR: #1894". Hope this clarifies things! Let me know if you have any other questions about how the learning system works. 🐰💭 🧠 Learnings used |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@adithya-naik Could you also add tests for this newly created component - ShowMoreButton?
@kasya as i mentioned I don't know how to add test cases as iam a beginner . |
Co-authored-by: Kate Golovanova <[email protected]>
|
Also, could you please add the gssoc25 label along with the existing one? That’ll help track contributions better. Thanks! |
…thya-naik/Nest into pr/adithya-naik/1894
|
The issue has the label - the PR doesn't need to have it. |
|
Thanks for the clarification, @kasya! 🙌 |
3 participants
📝 Pull Request Description
Description:
This PR fixes the misalignment of the "Show more / Show less" button in the Top Contributors section on the Projects page.
The button was previously off alignment compared to other elements in the card, as described in issue #1329.
Changes Made:
Related Issue:
Closes #1329
✅ Checklist: