Smooth animation for expanding/collapsing repositories list#1069
Smooth animation for expanding/collapsing repositories list#1069rishyym0927 wants to merge 11 commits intoOWASP:mainfrom
Conversation
…tor code structure
Summary by CodeRabbit
WalkthroughThe pull request introduces several changes. A new custom hook, Changes
Suggested labels
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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: 1
🧹 Nitpick comments (3)
frontend/src/hooks/useExpandableList.ts (1)
1-31: Great hook implementation for handling expandable lists!The
useExpandableListhook nicely centralizes the expandable list logic and animation handling. The generic type parameter makes this hook versatile and reusable across different components.A few suggestions:
- Consider extracting the animation duration (500ms) into a constant to ensure it stays in sync with your CSS animations
- The timeout in
toggleShowAll()should have a cleanup function like you did inuseEffect()+ const ANIMATION_DURATION = 500; // Match this with CSS animation duration useEffect(() => { // ... - }, 500) + }, ANIMATION_DURATION) // ... }, [showAll, animatingOut, items, maxInitialDisplay]) const toggleShowAll = () => { if (showAll) { setAnimatingOut(true) - setTimeout(() => setShowAll(false), 50) + const timer = setTimeout(() => setShowAll(false), 50) + return () => clearTimeout(timer) } else { setShowAll(true) } }frontend/src/components/ToggleableList.tsx (1)
21-36: Animation logic is complex but effectiveThe nested conditional rendering with multiple ternary operators works, but consider refactoring for readability:
- className={`rounded-lg border border-gray-400 px-2 py-1 text-sm transition-all duration-700 ease-in-out dark:border-gray-300 ${ - index >= limit - ? showAll - ? 'animate-fadeIn' - : animatingOut - ? 'animate-fadeOut' - : 'hidden' - : '' - }`} + className={`rounded-lg border border-gray-400 px-2 py-1 text-sm transition-all duration-500 ease-in-out dark:border-gray-300 ${ + getAnimationClass(index, limit, showAll, animatingOut) + }`}And add this helper function:
const getAnimationClass = (index, limit, showAll, animatingOut) => { if (index < limit) return ''; if (showAll) return 'animate-fadeIn'; if (animatingOut) return 'animate-fadeOut'; return 'hidden'; }Also, the transition duration (700ms) doesn't match the CSS animation duration (500ms) - they should be synchronized.
frontend/src/components/RepositoriesCard.tsx (1)
24-39: Animation logic needs alignment with CSSTwo issues to address:
- The transition duration (700ms) doesn't match the CSS animation duration (500ms)
- The complex nested ternary expressions reduce readability
- className={`transition-all duration-700 ease-in-out ${ - index >= 4 - ? showAll - ? 'animate-fadeIn' - : animatingOut - ? 'animate-fadeOut' - : 'hidden' - : '' - }`} + className={`transition-all duration-500 ease-in-out ${ + index >= 4 + ? (showAll ? 'animate-fadeIn' : animatingOut ? 'animate-fadeOut' : 'hidden') + : '' + }`}For even better readability, consider extracting this to a helper function like suggested for the ToggleableList component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/RepositoriesCard.tsx(1 hunks)frontend/src/components/ToggleContributors.tsx(3 hunks)frontend/src/components/ToggleableList.tsx(3 hunks)frontend/src/hooks/useExpandableList.ts(1 hunks)frontend/src/index.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (11)
frontend/src/components/ToggleableList.tsx (2)
15-15: Good implementation of the useExpandableList hook!The hook usage is clean and appropriate for this component.
39-53: Button state handling improvementGood job updating the button to respect animation state. The disabled attribute during animation prevents user interaction while the animation is in progress, enhancing the user experience.
frontend/src/components/RepositoriesCard.tsx (2)
18-18: Appropriate use of useExpandableList hookThe hook is implemented properly with a limit of 4 repositories.
45-58: Button state handling is well implementedThe button properly uses the toggleShowAll function and disables during animation, which prevents users from triggering multiple animations simultaneously.
frontend/src/components/ToggleContributors.tsx (7)
4-4: Great addition of the custom hook!Using the
useExpandableListhook aligns with the PR objective of centralizing expandable list functionality. This abstraction will help maintain consistency across the application.
20-23: Clean implementation of the expandable list hook.The hook provides all necessary functionality: visibility control, animation states, and toggle functionality. This is a significant improvement over managing these states separately in each component.
26-26: Explicit return value for empty state.Returning
nullfor empty contributors list is more explicit than the previous implementation. This is a small but good practice improvement.
33-33: Improved rendering logic with hook-provided values.Using
visibleItemsfrom the hook simplifies the component logic by delegating the filtering responsibility to the hook.
36-36: Well-implemented conditional styling for animations.The class name logic effectively handles different states (visible, animating in/out, hidden) based on the item index and hook state values. The transition duration of 700ms provides smooth animations as required.
73-75: Enhanced button behavior with animation states.The button is now properly disabled during animations and has visual feedback (opacity change) to indicate when it's in a transitioning state. This prevents users from triggering multiple animations simultaneously.
77-77: Improved button text toggle logic.Including the
animatingOutcondition in determining button text ensures the text doesn't change abruptly during the animation, providing a smoother user experience.
| @keyframes fadeIn { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateY(10px); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| } | ||
|
|
||
| @keyframes fadeOut { | ||
| from { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| to { | ||
| opacity: 0; | ||
| transform: translateY(10px); | ||
| } | ||
| } | ||
|
|
||
| @keyframes bounceSmall { | ||
| 0%, | ||
| 100% { | ||
| transform: translateY(0); | ||
| } | ||
| 50% { | ||
| transform: translateY(-4px); | ||
| } | ||
| } | ||
|
|
||
| .animate-fadeIn { | ||
| animation: fadeIn 0.5s ease-out; | ||
| } | ||
|
|
||
| .animate-fadeOut { | ||
| animation: fadeOut 0.5s ease-in; | ||
| } |
There was a problem hiding this comment.
Duplicate animation definitions with inconsistent timing
There are duplicate fadeIn and fadeOut keyframes in the file. The original animations (lines 121-141) have a duration of 0.3s while these new ones use 0.5s.
Suggested improvements:
- Remove the duplicates and enhance the existing animations with the translateY property
- Standardize the animation durations across the application
- Consider using CSS variables for animation durations to maintain consistency
- @keyframes fadeIn {
- from {
- opacity: 0;
- transform: translateY(10px);
- }
- to {
- opacity: 1;
- transform: translateY(0);
- }
- }
-
- @keyframes fadeOut {
- from {
- opacity: 1;
- transform: translateY(0);
- }
- to {
- opacity: 0;
- transform: translateY(10px);
- }
- }
/* Modify existing animations at lines 121-151 instead */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @keyframes fadeIn { | |
| from { | |
| opacity: 0; | |
| transform: translateY(10px); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translateY(0); | |
| } | |
| } | |
| @keyframes fadeOut { | |
| from { | |
| opacity: 1; | |
| transform: translateY(0); | |
| } | |
| to { | |
| opacity: 0; | |
| transform: translateY(10px); | |
| } | |
| } | |
| @keyframes bounceSmall { | |
| 0%, | |
| 100% { | |
| transform: translateY(0); | |
| } | |
| 50% { | |
| transform: translateY(-4px); | |
| } | |
| } | |
| .animate-fadeIn { | |
| animation: fadeIn 0.5s ease-out; | |
| } | |
| .animate-fadeOut { | |
| animation: fadeOut 0.5s ease-in; | |
| } | |
| /* Duplicate fadeIn and fadeOut keyframes removed. | |
| Modify existing animations at lines 121-151 instead */ | |
| @keyframes bounceSmall { | |
| 0%, | |
| 100% { | |
| transform: translateY(0); | |
| } | |
| 50% { | |
| transform: translateY(-4px); | |
| } | |
| } | |
| .animate-fadeIn { | |
| animation: fadeIn 0.5s ease-out; | |
| } | |
| .animate-fadeOut { | |
| animation: fadeOut 0.5s ease-in; | |
| } |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@rishyym0927 Thanks for working on this! Please update failing tests so that we can review and test this.
Yeah Im trying to fix and tune the tests !!! |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/SecondaryCard.tsx (1)
7-8: Consider using empty string as default for IDThe addition of the
idprop is well-implemented and enhances the component's flexibility. However, the default value of a single space" "doesn't conform to HTML ID standards, which don't allow spaces in IDs.- id = ' ', + id = '',Also applies to: 12-13, 14-17
frontend/src/components/RepositoriesCard.tsx (2)
25-34: Use a stable key to avoid re-render issues.The current usage of
indexas the key can lead to potential UI or re-rendering issues if the order of repositories changes. Consider using a unique identifier in each repository object (e.g.,repository.keyorrepository.id) as the key instead:- {visibleItems.map((repository, index) => ( - <div key={index} ...> + {visibleItems.map((repository) => ( + <div key={repository.key} ...>
45-49: Provide additional disabled-state user feedback.When
animatingOutis true, the button is disabled. You might consider updating the button text or style to indicate that it’s in a disabled (animating) state, helping users understand why the button is temporarily inactive.frontend/src/components/UserCard.tsx (1)
30-31: Clarify fallback logic for company, location, or email fields.You’re currently defaulting to show only the first non-empty of
company,location, or- <p className="mt-1 max-w-[250px] truncate text-sm text-gray-600 dark:text-gray-400 sm:text-base"> - {company || location || email} + <p className="mt-1 text-sm text-gray-600 dark:text-gray-400 sm:text-base"> + {[company, location, email].filter(Boolean).join(' · ')}
🛑 Comments failed to post (1)
frontend/src/components/MultiSearch.tsx (1)
49-53: 🛠️ Refactor suggestion
Maintain type safety for search hits.
Casting to multiple union types can obscure type-checking. To maximize clarity and catch potential errors, consider refining the type guard logic or using TypeScript’s discriminated unions for each index name.
|
@kasya Hey I have updated the testing files, cause the new animation is introduced thus there was a significant need to update the test files as the old ones will not work on it. |
arkid15r
requested review from
Dishant1804,
Rajgupta36,
abhayymishraa,
aramattamara and
kayashth1
|
Thanks for the PR @rishyym0927 i think we should animate the main container as well, because currently we are just animating the inner content and the outer container in which the data is being displayed is abruptly closing and opening without animation, we can animate that too |
|
Hey @Dishant1804, Thank you for your guidance! My approach was to generalize the animation for all "Show More" and "Show Less" buttons. Animating the entire box could be considered a separate issue, as it would make the animation more generalized, but we would still need to animate the inner content. In the future, we could explore using something like Framer for enhanced visuals. My primary goal, for now, was to animate only those specific sections. Appreciate your insights! 😊 |
|
@rishyym0927 the main goal is to focus on performance and reusability of the code at the same time i think we can implement the container animation here itself without framer motion library as of now, the implementation of inner container is good you can replicate similar kind of animation for the outer container as well that will be a better approach, addressing it here itself will help to stay in context of this issue of generalized animation for containers :) |
|
@Dishant1804 That sounds perfect to me! So basically, you’re suggesting that we implement the animation on the outer box itself and further generalize it for better reusability in the future. Would you like me to start working on this feature now, or should I wait for feedback from the other reviewers as well? |
Lets wait for the feedback from other reviewers as well, we need their inputs as well |
@rishyym0927, I agree with @Dishant1804's decision. Please implement a smooth height expansion animation when clicking "Show More" or "Show Less" instead of an sudden change. |
|
|
||
| {repositories.length > 4 && ( | ||
| <div className="mt-6 flex items-center justify-center text-center"> | ||
| <button |
There was a problem hiding this comment.
@rishyym0927 please use action button here, we have it in components, or if it does not fit with your requirements please create a separate button component that is modular and reusable, and one more thing we are using Chakra UI so if possible try to use components from the chakra ui library whenever possible to maintain consistency across the code base.
Thank you :)
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/index.css (1)
296-304: 🛠️ Refactor suggestionDuplicate and Inconsistent Fade Animations
The new
@keyframes fadeIn(lines 296–304) and@keyframes fadeOut(lines 305–314), along with the corresponding classes.animate-fadeInand.animate-fadeOut(lines 324–329), introduce updated animation timings (0.5s ease-out/in) and include a vertical translation effect. However, these definitions duplicate existing ones (defined earlier around lines 121–141 and 169–171) that use a 0.3s duration without the translate effect. This duplication can lead to maintenance challenges and unintended UI inconsistencies. It is recommended to consolidate the animations—either update the original definitions or remove them if the new animations are intended to be the single source of truth.Also applies to: 305-314, 324-329
🧹 Nitpick comments (1)
frontend/src/index.css (1)
315-323: Introduction of New BounceSmall AnimationThe
@keyframes bounceSmallanimation (lines 315–323) adds a subtle bouncing effect by translating the element upward by 4px at the midpoint. This effect can enhance visual feedback for interactive elements such as expandable lists. Ensure its usage is consistent with the overall design language and consider leveraging CSS variables for ease of future adjustments if needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/pages/ProjectDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/index.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/CardDetailsPage.tsx
- frontend/tests/unit/pages/RepositoryDetails.test.tsx
- frontend/tests/unit/pages/ProjectDetails.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
|
This has been stale for a while, I'm closing this. Feel free to reopen when you feel it's ready for review. |
5 participants
Resolves #1056
This PR introduces a global solution for expandable list animations, ensuring a smooth user experience when expanding and collapsing lists. It also refactors the project structure by introducing reusable hooks and global CSS improvements.
1️⃣ Created a Reusable Hook: useExpandableList
3️⃣ Global CSS Improvements
Before & After Improvements:
✅ Before:
Lists would expand/collapse abruptly.
Code for expand/collapse logic was duplicated in multiple components.
Inconsistent animation styles across the app.
✅ After:
Lists now animate smoothly when expanded/collapsed.
Centralized and reusable hook (useExpandableList).
Clean and maintainable component structure.
Video:
Recording.2025-03-10.210220.mp4