-
-
Notifications
You must be signed in to change notification settings - Fork 33
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/Add estimated matching time #4968
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces enhancements to the user interface for displaying time-related information about matching calculations across multiple languages and components. The changes include adding new translation keys in Catalan, English, and Spanish, updating GraphQL types and fragments, and modifying several React components to display the time since the last cluster matching synchronization. The implementation focuses on providing users with more transparent and up-to-date information about funding round updates. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (10)
src/components/views/donate/OneTime/EstimatedMatchingToast.tsx (2)
57-59
: Remove debug console.log statements.Debug statements should be removed before merging.
- console.log({ activeStartedRound }); - - console.log('activeQFRound', clusterMatchingSyncAt);
132-143
: Consider extracting duplicated styled component.The
ToolTipBellow
styled component is duplicated across multiple files. Consider moving it to a shared styled components file to maintain DRY principles.src/components/views/project/projectActionCard/QFSection.tsx (2)
78-81
: Remove commented out code and debug statements.Clean up the code by removing:
- Commented out code that's no longer needed
- Debug console.log statements
- // const { clusterMatchingSyncAt } = activeQFRound || {}; - console.log({ activeStartedRound }); - - console.log('activeQFRound', clusterMatchingSyncAt);
433-444
: Extract duplicated styled component.The
ToolTipBellow
styled component is duplicated. Consider moving it to a shared location, such as a common styled components file, to follow DRY principles.src/components/views/project/projectDonations/ProjectTotalFundCard.tsx (2)
227-231
: Improve accessibility by adding aria-label.The Flex component contains time-sensitive information but lacks accessibility attributes.
<Flex $justifyContent='space-between' $alignItems='center' $flexWrap={true} + aria-label="Estimated matching information" >
250-259
: Consider adding a loading state for time calculation.The time difference is directly rendered without handling potential loading states or errors.
<LastUpdate> + {clusterMatchingSyncAtDiff ? ( {formatMessage( { id: 'label.last_updated_ago', }, { time: clusterMatchingSyncAtDiff, }, )} + ) : ( + <span aria-label="Loading update time">...</span> + )} </LastUpdate>src/apollo/types/types.ts (1)
519-519
: Consider adding JSDoc comment for the new property.The
clusterMatchingSyncAt
property would benefit from documentation explaining its format and purpose.+/** ISO 8601 timestamp of the last cluster matching synchronization */ clusterMatchingSyncAt: string;
src/components/project-card/ProjectCard.tsx (2)
340-357
: Consider adding alt text translation for the clock icon.The clock icon's alt text should be translatable for consistency with other translated content.
<Image src='/images/icons/clock.svg' - alt='score' + alt={formatMessage({ id: 'component.icons.clock_alt' })} width={16} height={16} />
621-632
: Consider using theme colors for border.The hardcoded color
#121848
should be replaced with a theme color for consistency.- border-top: 1px solid #121848; + border-top: 1px solid ${neutralColors.gray[800]};lang/en.json (1)
70-70
: LGTM! Consider standardizing time unit abbreviation.Both translations look good and follow the existing patterns. For consistency across the platform, consider standardizing whether to use "min" or "minutes" for time units, as other translations in the file use the full word.
- "component.qf-section.estimated_time": "Estimated matching updated {time} min ago", + "component.qf-section.estimated_time": "Estimated matching updated {time} minutes ago", - "label.last_updated_ago": "Last updated: {time} min ago", + "label.last_updated_ago": "Last updated: {time} minutes ago"Also applies to: 560-560
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/icons/clock.svg
is excluded by!**/*.svg
📒 Files selected for processing (11)
lang/ca.json
(2 hunks)lang/en.json
(2 hunks)lang/es.json
(2 hunks)src/apollo/gql/gqlProjects.ts
(2 hunks)src/apollo/types/types.ts
(1 hunks)src/components/modals/HarvestAll.sc.tsx
(1 hunks)src/components/project-card/ProjectCard.tsx
(5 hunks)src/components/views/donate/OneTime/EstimatedMatchingToast.tsx
(5 hunks)src/components/views/project/projectActionCard/QFSection.tsx
(7 hunks)src/components/views/project/projectDonations/ProjectTotalFundCard.tsx
(6 hunks)src/helpers/time.ts
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/helpers/time.ts
[error] 71-71: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
src/components/modals/HarvestAll.sc.tsx (1)
90-91
: LGTM!The styling changes appropriately accommodate the new time information in the tooltip.
src/components/views/project/projectActionCard/QFSection.tsx (1)
446-451
: LGTM!The LastUpdate styled component is well-defined and appropriately styled.
src/components/views/project/projectDonations/ProjectTotalFundCard.tsx (2)
30-30
: LGTM! Time difference calculation implementation.The implementation correctly imports the helper function and calculates the time difference for display.
Also applies to: 57-57, 60-62
374-374
: LGTM! Styling implementation.The styling changes for EstimatedMatchingPrice and LastUpdate components are well-structured and maintain consistency with the design system.
Also applies to: 377-377, 426-435
src/apollo/gql/gqlProjects.ts (2)
24-24
: LGTM! Field addition to ProjectCoreFields fragment.The field is correctly added to the core fields fragment, ensuring consistent data fetching.
298-298
: LGTM! Field addition to ProjectBySlug query.The field is correctly added to the project query, maintaining consistency with the interface changes.
src/components/project-card/ProjectCard.tsx (1)
3-3
: LGTM! Time difference calculation setup.The implementation correctly imports required dependencies and sets up time difference calculation.
Also applies to: 40-40, 111-111, 114-116
lang/es.json (1)
70-70
: LGTM! The Spanish translations are accurate and well-formatted.The new translations for time-related messages are grammatically correct, maintain consistency with the English versions, and properly handle the {time} placeholder.
Also applies to: 558-558
lang/ca.json (1)
70-70
: LGTM! The new Catalan translations look good.The translations are grammatically correct, maintain consistent style with other translations, and properly handle the {time} placeholder variable.
Also applies to: 560-560
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.
@kkatusic great work man, is the backend ready for this on staging?
@mateodaza yes BE is ready, @CarlosQ96 added that time before I started this one. |
Summary by CodeRabbit
New Features
Improvements
Technical Updates
clusterMatchingSyncAt
field to project and funding round data structures