Fixed issue: 1082 Fixing the top contributors card to make it similar to recent issues card.#1137
Conversation
Summary by CodeRabbit
WalkthroughThis pull request updates both test and component files to revise how contributor details are rendered and verified. In tests, the "Top Contributors" section now focuses on visibility checks for text elements, and contributor identifiers have been updated from "user1" to "Contributor 1". In the components, the Changes
Suggested reviewers
✨ Finishing Touches
🪧 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 (1)
frontend/src/components/ToggleContributors.tsx (1)
1-78: Consider renaming the component file to match its exported component nameThe file is named
ToggleContributors.tsxbut exports a component calledTopContributors. This might lead to confusion for developers working on the codebase.Consider renaming the file to
TopContributors.tsxto match the exported component name for better code organization and clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/pages/ChapterDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/pages/Home.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/ToggleContributors.tsx (1)
frontend/src/types/contributor.ts (1) (1)
TopContributorsTypeGraphql(8:15)
🔇 Additional comments (9)
frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
90-90: LGTM! Contributor naming consistency update.The change from 'user1' to 'Contributor 1' improves naming clarity and consistency across tests.
Also applies to: 103-103
frontend/__tests__/unit/pages/CommitteeDetails.test.tsx (1)
83-83: LGTM! Consistent naming update aligned with other test files.The change from 'user1' to 'Contributor 1' maintains consistency with the updates in ChapterDetails.test.tsx.
Also applies to: 97-97
frontend/src/components/CardDetailsPage.tsx (1)
90-100: Good implementation of the contributor details rendering.The addition of the
renderDetailsprop enables displaying contribution counts in a standardized format, addressing the PR objective to make styling consistent with recent issues and releases sections.frontend/__tests__/e2e/pages/Home.spec.ts (1)
52-56: Improved test structure for top contributors.The updated test now properly verifies the visibility of contributor elements through a structured parent element reference, which better aligns with the component changes. This approach is more robust and maintainable.
frontend/src/pages/Home.tsx (1)
252-262: Good improvement to the Top Contributors componentThe changes successfully address PR objective #1082 by making the contributor cards consistent with the recent issues card. The increased
maxInitialDisplayvalue (from 6 to 9) provides better visibility of contributors on the main page.I like how the project name is now displayed in a standardized way, matching the pattern used for recent issues and releases.
frontend/src/components/ToggleContributors.tsx (4)
4-5: Good addition of necessary importsThe addition of
JSXtype from React and thecapitalizeutility function are appropriate for the component's enhanced functionality.
14-14: New prop enhances component flexibilityThe addition of the
renderDetailsprop properly addresses the PR objectives by allowing customized content rendering for each contributor. The type definition is well-structured and matches the component's usage in Home.tsx.Also applies to: 20-20
37-37: Improved responsive grid layoutThe updated grid layout with
gap-x-5 sm:grid-cols-2 md:grid-cols-3provides better spacing and responsiveness, matching the styling of other card components on the page.
38-54: Well-implemented clickable contributor cardsThe changes successfully implement the main objective of making the entire contributor card clickable (via the button wrapper) while maintaining a clean, structured layout. The use of:
- A test ID (
data-testid="top-contributor") improves testability- Properly structured nested divs for layout
- Capitalized names for consistent display
- Text overflow handling with
overflow-hidden text-ellipsis whitespace-nowrap- The
renderDetailsfunction to display custom contentAll work together to create a more consistent and user-friendly experience.
|
Hey @KaranNegi20Feb can you please review the PR. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
You still don't run what is suggested to run locally 🤷♂️
Please also don't close and open new PRs for the same issue -- we can work fine within the same PR.
| <button | ||
| key={index} | ||
| className="flex cursor-pointer items-center space-x-3 rounded-lg p-3 hover:bg-gray-200 dark:hover:bg-gray-700" | ||
| data-testid="top-contributor" |
There was a problem hiding this comment.
We don't use it in the app code.
There was a problem hiding this comment.
Ok ill change that
There was a problem hiding this comment.
Well i did that because it actually fails on the sonarqubecloud.
If you have any suggestions regarding this @arkid15r to guide me through that would be helpful 😊, Thank you.
There was a problem hiding this comment.
Upon reviewing the code I also saw this particular way is being used in the production code(main branch) aswell, that is why i opted for that 🥲
There was a problem hiding this comment.
This needs to be cleaned up. It's literally the single occurrence. Would you like to address it either here or in a separate issue?
There was a problem hiding this comment.
You can ignore SonarQube suggestions.
| <TopContributors contributors={topContributors} maxInitialDisplay={6} /> | ||
| <TopContributors | ||
| contributors={topContributors} | ||
| renderDetails={(item) => ( |
There was a problem hiding this comment.
Would passing just {item.contributionsCount} contributions be enough here?
There was a problem hiding this comment.
Yes, since we just want to show the contribution numbers by the top contributors in the projects page
There was a problem hiding this comment.
Can you move the rest (template) inside of the TopContributors component while using the actual content/value for props?
| <button | ||
| key={index} | ||
| className="flex cursor-pointer items-center space-x-3 rounded-lg p-3 hover:bg-gray-200 dark:hover:bg-gray-700" | ||
| data-testid="top-contributor" |
There was a problem hiding this comment.
This needs to be cleaned up. It's literally the single occurrence. Would you like to address it either here or in a separate issue?
| <button | ||
| key={index} | ||
| className="flex cursor-pointer items-center space-x-3 rounded-lg p-3 hover:bg-gray-200 dark:hover:bg-gray-700" | ||
| data-testid="top-contributor" |
There was a problem hiding this comment.
You can ignore SonarQube suggestions.
|
Yeah we can go ahead and create a new issue for that since I haven't tested that part much yet. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/ToggleContributors.tsx (2)
14-20: Consider using a union type fortypeprop for better type safety.The
typeprop currently uses a generic string type. For better type safety and developer experience, consider using a union type to restrict the possible values.- type: string + type: 'contributor' | 'company'This would prevent passing invalid values and provide better IDE autocompletion.
38-61: Improved UX with clickable contributor cards and conditional rendering.Making the entire card clickable as a button element improves user experience by providing a larger click target, which aligns with the PR objectives. The conditional rendering based on the
typeprop enables flexible display of either contribution count or project name.Consider adding an aria-label to improve accessibility:
<button key={index} onClick={() => navigate(`/community/users/${item.login}`)} className="mb-4 w-full rounded-lg bg-gray-200 p-4 dark:bg-gray-700" + aria-label={`View profile of ${capitalize(item.name) || capitalize(item.login)}`} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(2 hunks)frontend/src/pages/Home.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/pages/Home.tsx
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/ToggleContributors.tsx (1)
frontend/src/types/contributor.ts (1) (1)
TopContributorsTypeGraphql(8-15)
🔇 Additional comments (3)
frontend/src/components/ToggleContributors.tsx (3)
37-37: Good responsive grid layout improvement.Changing to a responsive grid layout with multiple columns on larger screens improves space utilization and creates a more visually appealing display.
46-49: Good use of the capitalize utility function.Using the
capitalizeutility function for displaying user names ensures consistent text formatting across the application.
53-57: Effective conditional rendering based on context.The conditional rendering based on the
typeprop is well-implemented and provides appropriate information in different contexts:
- For 'contributor' type: Shows contribution count
- For other types (like 'company'): Shows project name
This matches the PR objective of displaying only the project name without a link on the main page.
|
Hey @arkid15r, I made the required changes. You can now review the changes. Thank you. |
|
… to recent issues card. (OWASP#1137) * correction after review * resolved review * review corrections --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Fixed Issues:#1082
The Links in top contributors of home page does not redirect to the correct URL and fixing expandable clicking area.
Changes made:
make the entire highlighted contributor card area clickable (link to OWASP Nest user details page)
show just a project name (no link) on the main page while keeping the contribution count on the project details page
have the link the same style as with recent issues / recent releases for both main/project details page
Screen.Recording.2025-03-18.at.6.10.23.AM.mov
Screen.Recording.2025-03-18.at.6.10.42.AM.mov