WIP: Improve back option #4506
WIP: Improve back option #4506
Conversation
now I need to go through the all variables and states to check if something is missing
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes involve updates to dependency versions in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectsIndex
participant API
User->>ProjectsIndex: Scroll to bottom
ProjectsIndex->>API: fetchProjects(pageParam)
API-->>ProjectsIndex: Return new project data
ProjectsIndex->>User: Display new projects
Possibly related PRs
Suggested reviewers
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 (
|
|
@MohammadPCh just don't forget this :) |
|
Thanks @kkatusic! I filtered the QF projects but the number of total |
![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
Outside diff range and nitpick comments (1)
src/components/views/donate/DonationCard.tsx (1)
97-103: Approve commenting out the code for now, but track the feature refinement separately.Commenting out the
useEffectlogic that sets the default tab based on project type is a good temporary measure while the feature is being polished. It prevents unintended behavior for Endaoment projects.However, to ensure this feature is reintroduced properly, consider creating a separate issue or task to track its refinement and re-enablement. This will help maintain visibility on the intended behavior without cluttering the current code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
public/images/banners/qf-round/giv-palooza.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (5)
- package.json (2 hunks)
- src/components/project-card/ProjectCard.tsx (1 hunks)
- src/components/views/donate/DonationCard.tsx (2 hunks)
- src/components/views/project/ProjectGIVbackToast.tsx (2 hunks)
- src/wagmiConfigs.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- src/components/project-card/ProjectCard.tsx
Additional comments not posted (4)
src/wagmiConfigs.ts (1)
31-33: LGTM!The addition of the
safeconnector with theallowedDomainsproperty is a great enhancement to the wallet connectivity options. It improves user experience and security by allowing users to connect through the Safe wallet, while restricting connections to the Safe application hosted atapp.safe.global.src/components/views/donate/DonationCard.tsx (1)
56-56: LGTM!The
isEndaomentProjectvariable is a clear and concise way to determine if the project is an Endaoment project based on the organization label. The naming is intuitive and the logic is straightforward.src/components/views/project/ProjectGIVbackToast.tsx (2)
330-330: LGTM!Changing the width from a fixed value to
100%is a good practice for improving responsiveness. This allows the button wrapper to adapt to the width of its parent container, enhancing the layout across different screen sizes.
357-357: Verify the layout on larger screens.Updating the media query breakpoint from
tablettolaptopLis a design decision that aims to improve the user interface on larger devices. This change implies that the flex-direction will change from column to row at a larger screen size compared to the previous breakpoint.Please ensure that the new breakpoint aligns with the intended design and provides an optimal user experience across different screen sizes. Verify the layout on larger screens to confirm that the change achieves the desired outcome.
Found some time again to check this, when you @alireza-sharifpour found time please can you review it again? thx |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/components/views/projects/ProjectsIndex.tsx (1)
150-172: Remove console logs before merging.The code segment looks good overall, but there are a couple of console logs that should be removed before merging to production:
console.log('lastPage', lastPage); console.log('someData', someData);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/views/projects/ProjectsIndex.tsx (10 hunks)
Additional comments not posted (5)
src/components/views/projects/ProjectsIndex.tsx (5)
Line range hint
1-12:
Line range hint
25-46:
58-65: LGTM!The
Pageinterface is a good addition to provide a structured type for the data returned by thefetchProjectsfunction. It improves type safety and code readability.
68-81: LGTM!The code segment properly initializes the necessary state variables and hooks for the component. The use of
useQueryClientand the initialization oftotalPagesandtotalCountbased on the_totalCountprop are appropriate.
99-138: LGTM!The
fetchProjectsfunction is well-structured and handles the data fetching logic effectively. It constructs the necessary variables for the GraphQL query, updates the relevant state variables with the fetched data, and returns an object that follows thePageinterface. The use ofuseCallbackfor memoization is a good optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/project-card/ProjectCard.tsx (4)
Line range hint
130-131: Consider optimizing theuseEffectdependencyThe
useEffecthook now depends on the entirepropsobject, which might cause unnecessary re-calculations when unrelated props change. Consider specifying only the relevant props in the dependency array to optimize performance.For example, if only certain props affect the recurring donation calculations, you could destructure them:
const { id, activeStartedRound } = props; useEffect(() => { // ... calculation logic }, [id, activeStartedRound]);Could you clarify which specific prop changes should trigger this recalculation? This will help in determining the most appropriate dependencies for the effect.
Line range hint
301-301: Address TODO: Add recurring donation amountThere's a TODO comment indicating that the recurring donation amount should be added to the total donation amount for active rounds. This functionality seems to be missing, which could lead to inaccurate donation totals being displayed.
Could you provide an update on the status of this feature? If it's still pending, consider creating a separate issue to track this enhancement and ensure it's not overlooked.
Would you like me to create a GitHub issue for tracking this TODO item?
Line range hint
130-131: Consider moving data fetching logic to a custom hookThe component is directly fetching recurring donations data using the Apollo client. To improve separation of concerns and potentially optimize performance, consider moving this data fetching logic to a custom hook.
For example:
const useRecurringDonations = (projectId, startDate, endDate) => { const [recurringDonations, setRecurringDonations] = useState([]); useEffect(() => { const fetchData = async () => { // Fetch logic here }; fetchData(); }, [projectId, startDate, endDate]); return recurringDonations; };This approach would make the component more focused on rendering and improve testability. It also allows for easier reuse of this data fetching logic in other components if needed.
Line range hint
1-686: Consider refactoring for improved maintainabilityThe
ProjectCardcomponent is quite large and handles multiple responsibilities, including data fetching, complex rendering logic, and styling. To improve maintainability and testability, consider the following refactoring suggestions:
Break down the component into smaller, more focused sub-components. For example, you could create separate components for the card header, body, and footer.
Move styled components to a separate file (e.g.,
ProjectCard.styles.ts) to improve organization and reduce the size of the main component file.Consider using a custom hook for managing the component's state and side effects, separating the logic from the rendering.
If possible, move some of the complex calculations (e.g., for donations and matching) to utility functions that can be easily tested in isolation.
These changes would make the code more modular, easier to understand, and simpler to maintain and test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/project-card/ProjectCard.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/components/project-card/ProjectCard.tsx (1)
181-181: LGTM: Addingidattribute improves accessibility and targetingThe addition of the
idattribute to theCardBodycomponent is a good practice. It enhances accessibility and makes it easier to target specific project cards for styling or scripting purposes. Using the project's slug in theidensures uniqueness across different cards.
There was a problem hiding this comment.
@kkatusic Thanks, this is great! My suggestion would be to update the fetchProjects function to return the appropriate data in a way that allows the returned object to be used as state. That way, we won’t need to set separate states like setTotalPages, setTotalCount, and setFilteredProjects. Once that's done, we could move this function to the service file for better organization.
|
thx @MohammadPCh , changes have been made, please check again ;) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/components/views/projects/ProjectsIndex.tsx (1)
238-248: Safeguard against null references in scrolling logicWhen scrolling to the last clicked project, if
document.getElementById(lastProjectClicked)returnsnull, accessingoffsetTopwill result inundefined, which may cause unexpected scrolling behavior.Add a check to ensure the element exists before attempting to scroll:
if (lastProjectClicked) { const element = document.getElementById(lastProjectClicked); + if (element) { window.scrollTo({ top: element.offsetTop, behavior: 'smooth', }); + } localStorage.removeItem('lastProjectClicked'); }This prevents potential errors if the element is not found in the DOM.
Update scroll to clicked proejct
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
src/components/views/projects/ProjectsIndex.tsx (1)
267-270: Improve accessibility by using interactive elementsUsing a
divwith anonClickhandler may not be accessible to all users. Consider using a clickable element like a<button>or an<a>tag to enhance accessibility.Apply this diff to replace the
divwith anatag:- <div + <a key={project.id} id={project.slug} onClick={() => handleProjectClick(project.slug) } + href="#" > <ProjectCard project={project} order={idx} /> - </div> + </a>Remember to prevent the default action if using an anchor tag:
onClick={(e) => { + e.preventDefault(); handleProjectClick(project.slug); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/views/projects/ProjectsIndex.tsx (6 hunks)
- src/components/views/projects/constants.ts (1 hunks)
- src/components/views/projects/services.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/views/projects/constants.ts
🧰 Additional context used
Summary by CodeRabbit
New Features
Improvements
useInfiniteQueryhook.Bug Fixes