- Notifications
You must be signed in to change notification settings - Fork95
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Implement Dynamic Labels & Repository Languages in the Contribute Page#1126
base:main
Are you sure you want to change the base?
Conversation
Summary by CodeRabbit
WalkthroughThis change introduces two new properties, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/Card.tsx(4 hunks)frontend/src/pages/Contribute.tsx(3 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/modal.ts(1 hunks)
🔇 Additional comments (6)
frontend/src/types/modal.ts (1)
13-14:Interface extension looks goodThe addition of optional
labelsandrepository_languagesproperties to theModalPropsinterface is clean and maintains backward compatibility. Using string arrays for these properties is appropriate for representing collections of labels and programming languages.frontend/src/types/card.ts (1)
14-15:Interface extension is consistentThe addition of optional
labelsandrepository_languagesproperties to theCardPropsinterface mirrors the changes in theModalPropsinterface, maintaining consistency across the codebase. The types are appropriate for the data being represented.frontend/src/pages/Contribute.tsx (2)
56-57:Properly connecting issue data to Card componentThe code correctly passes the labels and repository languages from the issue object to the Card component.
70-71:Maintaining UI consistency between Card and ModalGood job passing the same properties to both the Card and Modal components, ensuring consistent information display across UI elements.
frontend/src/components/Card.tsx (2)
32-33:Props declaration matches interfaceThe component props declaration correctly includes the new properties defined in the CardProps interface.
121-133:Repository languages implementation looks goodThe implementation for displaying repository languages is well-structured:
- It properly checks both existence and length before rendering
- The styling is consistent with the app's design system
- It supports both light and dark modes with appropriate color contrasts
- The flex-wrap layout ensures good responsiveness on various screen sizes
adding null check for the labelsCo-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Hey@arkid15r do i need to make any change in this pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice update! Left some comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We used to have these labels before, so thanks for adding those back.
One of the features they had is that they were clickable with the label/topic being added to a search field and fetching filtered records.
Do you think you could add that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"Hey@kasya, that's a great idea! Thanks for suggesting it. Since implementing this would require some modifications to the backend, I think it would be best to address it in a new issue. I'm currently working on the backend and will make sure to include this feature in future updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Card.tsx (1)
114-129:Consider making labels clickable for filteringPer the past review comments, there was a feature request to make labels clickable for filtering content. While you've responded that this requires backend changes, consider preparing the UI for this functionality.
You could modify the label spans to be wrapped in a button or link component with an onClick handler that would be implemented later:
- <span+ <button key={`label-${index}`} className={`rounded-md px-3 py-1 text-sm font-medium ${ isMobile ? 'bg-sky-200 text-sky-900' : 'bg-sky-100 text-sky-800 dark:bg-sky-900 dark:text-sky-300' }`}+ onClick={() => {/* To be implemented */}} > {label}- </span>+ </button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Card.tsx(4 hunks)frontend/src/pages/Contribute.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Contribute.tsx
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/Card.tsx (3)
frontend/src/types/card.ts (1)
CardProps(11-26)frontend/src/utils/constants.ts (1)
desktopViewMinWidth(90-90)frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-54)
🔇 Additional comments (10)
frontend/src/components/Card.tsx (10)
30-31:Props added for enhanced information displayThe addition of the
labelsandrepository_languagesprops aligns with the PR objectives to support dynamic labels and repository languages on the Contribute page.
36-36:Simplified mobile detection functionGood refactoring - extracting the mobile detection logic into a named function improves readability.
42-46:Responsive container layout with conditional paddingThe container's padding now adapts based on the mobile state, providing better spacing on different screen sizes.
48-72:Simplified title display with direct link wrappingThe title is now wrapped directly in a Link component with proper styling. This is a cleaner approach than previous implementations.
74-86:Improved icon display logicThe condition for displaying icons has been simplified to a more readable form, checking if any icon keys exist in the icons object.
90-97:Enhanced project link stylingThe project link styling has been updated with more specific classes for better visibility in both light and dark modes.
101-112:Repository languages display implementationThe implementation for displaying repository languages looks good. The conditional rendering ensures the section only appears when languages are available.
114-129:Add null check for labels arrayWhile the
labels?.length > 0check prevents rendering when labels is empty, there's a past review comment that should be addressed. The render function inside still useslabels.map()without a null check, which could cause a runtime error.Apply this change:
- {labels.map((label, index) => (+ {labels?.map((label, index) => ( <span key={`label-${index}`} className={`rounded-md px-3 py-1 text-sm font-medium ${
114-129:Responsive styling for labels based on mobile stateGood implementation of conditional styling based on mobile state. This ensures labels look appropriate on both desktop and mobile views.
131-165:Improved layout for social links and action buttonThe layout for social links and action button has been restructured into a flex container with proper justification, improving the overall UI organization.
Recording.2025-03-30.173734.mp4Hey@kasya I have made the changes requested!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few more requests ⬇️
| </div> | ||
| )} | ||
| {labels?.length > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Labels background changes on smaller screens 🤔
Dark theme:
Light theme:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/components/Card.tsx (2)
118-133:⚠️ Potential issueFix inconsistent label styles between mobile and desktop views.
Labels have different background and text colors depending on screen size, which creates visual inconsistency when switching between views. This was previously noted in review comments.
Apply this fix to maintain consistent styling across all screen sizes:
className={`rounded-md px-3 py-1 text-sm font-medium ${- isMobile- ? 'bg-sky-200 text-sky-900'- : 'bg-sky-100 text-sky-800 dark:bg-sky-900 dark:text-sky-300'+ 'bg-sky-100 text-sky-800 dark:bg-sky-900 dark:text-sky-300' }`}
146-168:⚠️ Potential issueAdjust action button positioning to match previous layout.
The action button should be positioned at the far right of the card, as per previous feedback from reviewers. Currently, it's grouped with social links which doesn't match the expected design.
Apply this change to fix the layout:
- <div className="flex w-full items-center justify-between">+ <div className="flex w-full items-center justify-between"> {social && social.length > 0 && (- <HStack mt={2}>+ <HStack mt={2} className="flex-grow"> {social.map((item) => ( <Link key={`${item.title}-${item.url}`} href={item.url} rel="noopener noreferrer" display="flex" alignItems="center" gap={2} > <FontAwesomeIcon icon={getSocialIcon(item.url)} className="h-5 w-5" /> </Link> ))} </HStack>+ ) || <div className="flex-grow"></div>}- )} <ActionButton tooltipLabel={tooltipLabel} url={button.url} onClick={button.onclick}> {button.icon} {button.label} </ActionButton> </div>This ensures the button is always at the far right, regardless of whether social links are present or not.
🧹 Nitpick comments (1)
frontend/src/components/Card.tsx (1)
129-130:Future enhancement: Make labels clickable for filtering.In a previous discussion, it was mentioned that labels should be clickable and filter content. While not implemented in this PR (as it requires backend changes), consider adding this functionality in a future update.
Would you like help creating an issue to track the implementation of clickable labels for filtering?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Card.tsx(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/Card.tsx (4)
frontend/src/types/card.ts (1)
CardProps(11-26)frontend/src/utils/constants.ts (1)
desktopViewMinWidth(91-91)frontend/src/utils/data.ts (1)
Icons(66-107)frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-54)
🔇 Additional comments (6)
frontend/src/components/Card.tsx (6)
30-31:Props added successfully to support dynamic labels and repository languages.These new props align with the PR objectives to enhance the Card component with the ability to display labels and repository languages.
36-36:Clean refactoring of mobile detection logic.The simplified function improves readability while maintaining the same functionality.
42-46:Improved responsive container styling.Good implementation of conditional padding based on screen size, ensuring appropriate spacing on both mobile and desktop views.
105-116:Good implementation of repository languages display.The code properly handles the conditional rendering with null checks and applies appropriate styling for the language tags that works in both light and dark themes.
118-118:Good implementation of null check for labels array.The optional chaining operator properly guards against potential runtime errors when labels is undefined or null, addressing previous feedback.
71-71:Good UI/UX improvements throughout the component.Several small enhancements improve the component:
- Smooth color transition for the title
- More robust icon display logic with better conditional checks
- Enhanced project link styling for better visibility
These changes result in a more polished user experience.
Also applies to: 78-90, 94-98
Resolves#1108
Title: 🏗️ Implement Dynamic Labels & Repository Languages in the Contribute Page 🚀
Description:
This PR enhances the
Cardcomponent and updates theContribute Page to support dynamic labels and repository languages. These changes improve the information displayed for each project, making it easier for users to identify relevant details at a glance.Key Changes:
CardComponent:labelsandrepository_languagesas new props.CardPropsandModalPropsto include the new properties.Why This is Needed:
Cardcomponent displays structured data.Testing & Validation:
✔️ Verified UI updates in different screen sizes.
✔️ Ensured TypeScript type safety for new props.
Recording.2025-03-17.165756.mp4