Skip to content

Conversation

n3-rd
Copy link

@n3-rd n3-rd commented Aug 26, 2025

User description

This pull request introduces several improvements to the UI/UX and code structure of the EcosystemGrid, FAQContainer, and Header components, with a particular focus on smoother transitions, improved mobile navigation, and enhanced theming support. The most notable changes include the addition of fade animations for grid transitions, a major refactor of the mobile navigation menu for accessibility and responsiveness, and the introduction of dynamic theme-based text color variables. Below are the key changes grouped by theme:

EcosystemGrid Enhancements

  • Added fade-in/out animations to grid transitions using new isFading state and corresponding SCSS, resulting in smoother UI updates when changing tags. Highlighted and non-highlighted items are now separated for better visual structure. (containers/EcosystemGrid/EcosystemGrid.jsx, containers/EcosystemGrid/EcosystemGrid.module.scss) [1] [2] [3] [4] [5]
  • Switched from <img> to Next.js <Image> component for better image optimization and performance in both highlight and card grids. (containers/EcosystemGrid/EcosystemGrid.jsx) [1] [2]

FAQContainer Improvements

  • Improved FAQ expand/collapse animation with smooth transitions and max-height/opacity changes, and made the answer text color theme-dependent. Also, the FAQ now properly updates when the type prop changes. (containers/Faq-container/FaqContainer.jsx, containers/Faq-container/FaqContainer.module.scss) [1] [2] [3] [4]

Header/Mobile Navigation Refactor

  • Refactored the mobile navigation menu for accessibility and responsiveness: the menu now uses a fixed, animated overlay with improved transitions, closes on scroll, and disables body scrolling when open. Theme toggling is accessible via keyboard and available in both desktop and mobile menus.
image

(containers/Header/Header.jsx, containers/Header/header.module.scss) [1] [2] [3] [4] [5] [6] [7]

Theming and Visual Consistency

  • Introduced a new CSS variable --answer-text-color to enable dynamic answer text color based on theme, and updated relevant SCSS and theme configuration. This variable is now used across FAQ answers and LighthouseSuit descriptions for consistent theming. (utils/services/theme.js, containers/Faq-container/FaqContainer.module.scss, containers/LighthouseSuit/LighthouseSuit.module.scss) [1] [2] [3] [4]

Minor Fixes and Cleanups

  • Fixed initialization values and dependencies for several state variables to avoid potential bugs and improve clarity. (containers/Header/Header.jsx, containers/Faq-container/FaqContainer.jsx) [1] [2]

These changes collectively improve the user experience, accessibility, and maintainability of the codebase.


PR Type

Enhancement


Description

  • Added fade animations to EcosystemGrid for smoother transitions

  • Refactored mobile navigation with improved accessibility and animations

  • Enhanced FAQ animations with smooth expand/collapse effects

  • Added dynamic theme-based text color variables


Diagram Walkthrough

flowchart LR
  A["Theme System"] --> B["Dynamic Colors"]
  C["EcosystemGrid"] --> D["Fade Animations"]
  C --> E["Next.js Image"]
  F["Mobile Menu"] --> G["Smooth Transitions"]
  F --> H["Accessibility"]
  I["FAQ Container"] --> J["Expand/Collapse"]
  B --> I
  B --> K["LighthouseSuit"]
Loading

File Walkthrough

Relevant files
Configuration changes
theme.js
Add dynamic answer text color theme variable                         

utils/services/theme.js

  • Added --answer-text-color CSS variable for theme-based text colors
+5/-0     
Enhancement
EcosystemGrid.module.scss
Add fade animations for grid transitions                                 

containers/EcosystemGrid/EcosystemGrid.module.scss

  • Added fade transition styles for grid and card elements
  • Implemented opacity and transform animations for smooth transitions
+19/-0   
FaqContainer.module.scss
Enhance FAQ animations and theme integration                         

containers/Faq-container/FaqContainer.module.scss

  • Updated answer text color to use theme variable
  • Added smooth expand/collapse animations with max-height and opacity
  • Enhanced answer box transitions with transform effects
+14/-1   
header.module.scss
Redesign mobile navigation with animations                             

containers/Header/header.module.scss

  • Fixed syntax error in CSS selector
  • Completely redesigned mobile menu with modern animations
  • Added responsive breakpoints for tablet and mobile views
  • Implemented smooth transitions and staggered animations
+166/-21
LighthouseSuit.module.scss
Apply theme-based colors to text elements                               

containers/LighthouseSuit/LighthouseSuit.module.scss

  • Updated description and feature text colors to use theme variables
+2/-2     
EcosystemGrid.jsx
Add fade animations and optimize images                                   

containers/EcosystemGrid/EcosystemGrid.jsx

  • Added fade animation state management with useEffect
  • Replaced tags with Next.js components
  • Separated highlighted and non-highlighted items for better structure
  • Implemented smooth transitions between filter changes
+110/-83
FaqContainer.jsx
Improve FAQ state management and animations                           

containers/Faq-container/FaqContainer.jsx

  • Changed initial isOpen state from 0 to null
  • Added type dependency to useEffect for proper re-rendering
  • Enhanced answer box with conditional CSS classes for animations
  • Fixed apostrophe encoding in text content
+14/-11 
Header.jsx
Refactor mobile navigation with accessibility improvements

containers/Header/Header.jsx

  • Refactored mobile menu with improved state management
  • Added scroll-based menu closing functionality
  • Enhanced accessibility with proper ARIA labels and keyboard navigation
  • Implemented body scroll lock when menu is open
  • Consolidated theme toggle functionality
+96/-79 

n3-rd added 4 commits August 25, 2025 13:48
…Introduced useEffect for dynamic filtering of highlighted and non-highlighted items based on activeTag. Updated styles for smooth transitions on item visibility.
…ed initial state of isOpen to null, added useEffect dependency on type, and enhanced answer box visibility with CSS transitions for smoother animations.
Copy link

vercel bot commented Aug 26, 2025

@n3-rd is attempting to deploy a commit to the lighthouse-storage Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Error Handling

The code opens external URLs without error handling when clicking on social media icons. If the URL is invalid or unavailable, there's no fallback behavior or user feedback.

  className="ptr"
  onClick={() => {
    window.open(item.website, "_blank");
  }}
>
Accessibility Issue

The mobile menu toggle button lacks proper ARIA attributes. While role and aria-label were added to the theme toggle, the menu toggle button should also have appropriate accessibility attributes.

  <RiCloseLine
    color="#ffff"
    size={27}
    onClick={() => setToggleMenu(false)}
  />
) : (
  <RiMenuFill
    color="#ffff"
    size={27}
    onClick={() => setToggleMenu(true)}
  />
)}
Performance Concern

The fade animation implementation uses nested setTimeout functions which could cause performance issues. Consider using React's useTransition or a dedicated animation library for smoother transitions.

useEffect(() => {
  setIsFading(true);
  const fadeTimeout = setTimeout(() => {
    const allFiltered = ecosystemData.filter((item) =>
      item.tags.includes(activeTag)
    );

    const highlights = allFiltered.filter((item) => item.image);
    const nonHighlights = allFiltered.filter((item) => !item.image);

    setHighlightItems(highlights);
    setFilteredItems(nonHighlights);
    setTimeout(() => {
      setIsFading(false);
    }, 50);
  }, 500);

  return () => clearTimeout(fadeTimeout);
}, [activeTag]);

Copy link

codiumai-pr-agent-free bot commented Aug 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix scroll event handler
Suggestion Impact:The commit implemented the core of the suggestion by removing scrollTop from the dependency array and properly tracking the previous scroll position, though using useRef instead of a local variable

code diff:

+  const prevScrollRef = useRef(scrollTop);
+
   useEffect(() => {
     const onScroll = (e) => {
       const newScrollTop = e.target.documentElement.scrollTop;
       setScrollTop(newScrollTop);
-      setScrolling(newScrollTop > scrollTop);
+      setScrolling(newScrollTop > prevScrollRef.current);
       // This is done to close the mobile menu when scrolling
-      if (Math.abs(newScrollTop - scrollTop) > 50) {
+      if (Math.abs(newScrollTop - prevScrollRef.current) > 50) {
         setToggleMenu(false);
       }
+      prevScrollRef.current = newScrollTop;
     };
     window.addEventListener("scroll", onScroll);
     return () => window.removeEventListener("scroll", onScroll);
-  }, [scrollTop]);
+  }, []);

The scroll event handler depends on scrollTop which changes on every scroll,
causing the effect to run repeatedly and creating new event listeners without
properly removing old ones. This leads to memory leaks and performance issues.

containers/Header/Header.jsx [61-73]

 useEffect(() => {
+  let prevScrollTop = scrollTop;
   const onScroll = (e) => {
     const newScrollTop = e.target.documentElement.scrollTop;
     setScrollTop(newScrollTop);
-    setScrolling(newScrollTop > scrollTop);
+    setScrolling(newScrollTop > prevScrollTop);
     // This is done to close the mobile menu when scrolling
-    if (Math.abs(newScrollTop - scrollTop) > 50) {
+    if (Math.abs(newScrollTop - prevScrollTop) > 50) {
       setToggleMenu(false);
     }
+    prevScrollTop = newScrollTop;
   };
   window.addEventListener("scroll", onScroll);
   return () => window.removeEventListener("scroll", onScroll);
-}, [scrollTop]);
+}, []);

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a performance issue where the scroll event listener is re-registered on every scroll event, and proposes a valid fix using an empty dependency array.

Medium
Fix memory leak
Suggestion Impact:The commit implemented the suggestion by storing the inner setTimeout reference in a variable (unfadeTimeout) and clearing it in the cleanup function, preventing potential memory leaks

code diff:

+    let unfadeTimeout;
     const fadeTimeout = setTimeout(() => {
       const allFiltered = ecosystemData.filter((item) =>
         item.tags.includes(activeTag)
@@ -409,12 +410,15 @@
 
       setHighlightItems(highlights);
       setFilteredItems(nonHighlights);
-      setTimeout(() => {
+      unfadeTimeout = setTimeout(() => {
         setIsFading(false);
       }, 50);
     }, 500);
 
-    return () => clearTimeout(fadeTimeout);
+    return () => {
+      clearTimeout(fadeTimeout);
+      if (unfadeTimeout) clearTimeout(unfadeTimeout);
+    };

The nested setTimeout creates a potential memory leak. The inner setTimeout
isn't being cleared in the cleanup function, which could lead to state updates
after the component unmounts. Store both timeout references and clear them
properly.

containers/EcosystemGrid/EcosystemGrid.jsx [400-418]

 useEffect(() => {
   setIsFading(true);
   const fadeTimeout = setTimeout(() => {
     const allFiltered = ecosystemData.filter((item) =>
       item.tags.includes(activeTag)
     );
     
     const highlights = allFiltered.filter((item) => item.image);
     const nonHighlights = allFiltered.filter((item) => !item.image);
 
     setHighlightItems(highlights);
     setFilteredItems(nonHighlights);
-    setTimeout(() => {
+    
+    const unfadeTimeout = setTimeout(() => {
       setIsFading(false);
     }, 50);
+    
+    return () => clearTimeout(unfadeTimeout);
   }, 500);
 
   return () => clearTimeout(fadeTimeout);
 }, [activeTag]);

[Suggestion processed]

Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies a potential memory leak from an uncleared nested setTimeout, but the proposed improved_code is incorrect and would not fix the issue.

Low
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant