Skip to content
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.

Sign up for GitHub

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

Added Hovering effect to social icons in Footer issue -#2388 #2416

Closed
wants to merge 3 commits into from
Closed

Added Hovering effect to social icons in Footer issue -#2388 #2416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bf9707
Added box shadow in divs
Mahimatestgithub Oct 27, 2024
66aabfa
Added subscribe
Mahimatestgithub Oct 27, 2024
b4485c3
added hovering to social icons
Mahimatestgithub Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions src/User/components/Footer/Footer.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* Existing Footer Styles */
.footer {
background-color: #2e2e2e;
color: #fff;
Expand All @@ -9,13 +10,11 @@
.footer-top {
display: flex;
flex-direction: column;
/* gap: 20px; */
align-items: center;
}

.footer-top .logo img {
width: 80px;
/* Adjusted size for better visibility */
}

.footer-top .social-media {
Expand All @@ -33,7 +32,6 @@

.footer-top .social-icons a img {
width: 30px;
/* Adjusted size for consistency */
}

.footer-top .contact-info,
Expand All @@ -47,7 +45,6 @@
align-items: center;
justify-content: center;
gap: 10px;
/* Space between image and text */
margin: 5px 0;
color: #fff;
text-decoration: none;
Expand All @@ -60,7 +57,6 @@

.footer-top .address img {
width: 24px;
/* Adjusted size for better alignment */
height: 24px;
}

Expand Down Expand Up @@ -95,6 +91,28 @@
text-decoration: underline;
}

/* Social Media Icon Hover Effects */
.footer-top .social-icons a {
transition: transform 0.3s, filter 0.3s;
}

.footer-top .social-icons a:hover {
transform: scale(1.1);
}

.footer-top .social-icons a:nth-child(1):hover img {
filter: brightness(1.2) saturate(1.2);
}

.footer-top .social-icons a:nth-child(2):hover img {
filter: brightness(1.2) saturate(1.2);
}

.footer-top .social-icons a:nth-child(6):hover img {
filter: brightness(1.2) saturate(1.2);
}
Comment on lines +94 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor hover effects to use more maintainable selectors.

The current implementation has several issues:

  1. Using nth-child selectors makes the code fragile if icon order changes
  2. Duplicate filter values across selectors
  3. Missing documentation for which social media each selector targets

Consider using this more maintainable approach:

- /* Social Media Icon Hover Effects */
+ /* Social Media Icon Hover Effects - Base styles */
.footer-top .social-icons a {
  transition: transform 0.3s, filter 0.3s;
}

.footer-top .social-icons a:hover {
  transform: scale(1.1);
}

- .footer-top .social-icons a:nth-child(1):hover img {
-   filter: brightness(1.2) saturate(1.2);
- }
-
- .footer-top .social-icons a:nth-child(2):hover img {
-   filter: brightness(1.2) saturate(1.2);
- }
-
- .footer-top .social-icons a:nth-child(6):hover img {
-   filter: brightness(1.2) saturate(1.2);
- }

+ /* Individual social media hover effects */
+ .footer-top .social-icons a[href*="instagram"]:hover img,
+ .footer-top .social-icons a[href*="twitter"]:hover img,
+ .footer-top .social-icons a[href*="discord"]:hover img {
+   filter: brightness(1.2) saturate(1.2);
+ }
📝 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.

Suggested change
/* Social Media Icon Hover Effects */
.footer-top .social-icons a {
transition: transform 0.3s, filter 0.3s;
}
.footer-top .social-icons a:hover {
transform: scale(1.1);
}
.footer-top .social-icons a:nth-child(1):hover img {
filter: brightness(1.2) saturate(1.2);
}
.footer-top .social-icons a:nth-child(2):hover img {
filter: brightness(1.2) saturate(1.2);
}
.footer-top .social-icons a:nth-child(6):hover img {
filter: brightness(1.2) saturate(1.2);
}
/* Social Media Icon Hover Effects - Base styles */
.footer-top .social-icons a {
transition: transform 0.3s, filter 0.3s;
}
.footer-top .social-icons a:hover {
transform: scale(1.1);
}
/* Individual social media hover effects */
.footer-top .social-icons a[href*="instagram"]:hover img,
.footer-top .social-icons a[href*="twitter"]:hover img,
.footer-top .social-icons a[href*="discord"]:hover img {
filter: brightness(1.2) saturate(1.2);
}

Comment on lines +103 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

CSS selectors don't match the intended social icons

The current implementation has issues with the nth-child selectors:

  • Instagram is correctly targeted as nth-child(1)
  • Twitter/X is correctly targeted as nth-child(2)
  • Discord is nth-child(6), which is correct
  • However, the CSS is applying filter: brightness(1.2) saturate(1.2) to img elements, but the icons are implemented using FaInstagram, BsTwitterX, and FaDiscord components, not images

The hover effects won't work because:

  1. The selectors target img elements that don't exist
  2. The icons are rendered as SVG components, not images
🔗 Analysis chain

Verify hover effects target the correct social icons.

The current implementation uses nth-child selectors (1, 2, and 6), but it's unclear if these correspond to Instagram, Twitter, and Discord icons as mentioned in the PR objectives.

Let's verify the icon order in the JSX file:

Let me gather more information to see the complete social icons implementation:

Let me check the complete social icons section to see all icons:

Let me get the complete list of social icons to verify all entries:

Let me get one final check to see if there are any more social icons:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and display the social icons implementation in Footer.jsx
rg -l "Footer" --type jsx | xargs rg -A 10 "social-icons"

Length of output: 3392

Script:

#!/bin/bash
# Find all social icon links in Footer.jsx
rg -A 5 "social-icons" src/User/components/Footer/Footer.jsx

Length of output: 324

Script:

#!/bin/bash
# Get more context around social icons section
rg -B 2 -A 20 "social-icons" src/User/components/Footer/Footer.jsx

Length of output: 951

Script:

#!/bin/bash
# Get the complete social icons section including all icon entries
rg -B 2 -A 40 "social-icons" src/User/components/Footer/Footer.jsx

Length of output: 1663

Script:

#!/bin/bash
# Get an even larger context to ensure we see all social icons
rg -B 2 -A 60 "social-icons" src/User/components/Footer/Footer.jsx

Length of output: 2347


/* Responsive Styles */
@media (min-width: 768px) {
.footer-top {
flex-direction: row;
Expand Down Expand Up @@ -123,4 +141,4 @@
text-align: right;
justify-content: flex-end;
}
}
}
3 changes: 3 additions & 0 deletions src/User/components/Footer/Footer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ const Footer = () => {
</p>
</div>
<br />



<div className="social-media flex flex-col justify-center items-center">
<p className="text-sm text-center text-gray-400 sm:items-center underline">
SOCIALS:
Expand Down
6 changes: 6 additions & 0 deletions src/User/pages/Home/Home.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@
animation: smoothUpDown 2s infinite alternate ease-in-out;
}

.custom-shadow {
box-shadow: 0px 6px 20px rgba(60, 60, 60, 0.3); /* Dark gray, thick shadow */
border-radius: 8px;
}


@keyframes smoothUpDown {

0%,
Expand Down
37 changes: 32 additions & 5 deletions src/User/pages/Home/Home.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ const Home = () => {
const [suggestions, setSuggestions] = useState([]);
const navigate = useNavigate();

const [isSubscribed, setIsSubscribed] = useState(false);
const [email, setEmail] = useState("");

const handleSearch = (e) => {
const term = e.target.value;
setSearchTerm(term);
Expand All @@ -160,6 +163,17 @@ const Home = () => {
sectionRef.current.scrollIntoView({ behavior: "smooth" });
};


const handleSubscribe = (e) => {
e.preventDefault();
setIsSubscribed(true); // Show the success message
setTimeout(() => {
setIsSubscribed(false); // Hide the message
setEmail(""); // Clear the email input
}, 3000); // 3 seconds
};

Comment on lines +167 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add proper form validation and error handling.

The current implementation has several potential issues:

  1. Missing email validation
  2. No error handling
  3. No actual subscription logic (API call)
  4. Timeout-based state reset could cause race conditions if user submits multiple times

Consider implementing this improved version:

-const handleSubscribe = (e) => {
+const handleSubscribe = async (e) => {
   e.preventDefault();
+  if (!email || !email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) {
+    // Show error message
+    return;
+  }
+  try {
+    // Add loading state if needed
+    // await subscribeUser(email);
     setIsSubscribed(true);
-    setTimeout(() => {
-      setIsSubscribed(false);
-      setEmail("");
-    }, 3000);
+    setEmail("");
+    // Reset subscription state after delay, but only if still subscribed
+    const timeoutId = setTimeout(() => {
+      setIsSubscribed((current) => current ? false : current);
+    }, 3000);
+    // Clean up timeout if component unmounts
+    return () => clearTimeout(timeoutId);
+  } catch (error) {
+    // Handle error appropriately
+    console.error('Subscription failed:', error);
+  }
 };

Committable suggestion was skipped due to low confidence.


return (
<div className="bg-[#fff0e3ff]">
<main className="">
Expand Down Expand Up @@ -218,12 +232,14 @@ const Home = () => {
</h2>
<div className="grid grid-cols-2 sm:grid-cols-3 md:grid-cols-4 gap-3 sm:gap-4 md:gap-6">
{popularCategories.map((category, index) => (
<div key={index} className="custom-shadow">
<CategoryCard
key={index}
name={category.name}
image={category.image}
path={category.path}
/>
</div>
Comment on lines +235 to +242
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix duplicate key prop and verify layout impact.

  1. The key prop is unnecessarily duplicated on both the wrapper div and the CategoryCard
  2. Adding a wrapper div might affect the grid layout spacing

Apply this fix:

-<div key={index} className="custom-shadow">
-<CategoryCard
-  key={index}
-  name={category.name}
-  image={category.image}
-  path={category.path}
-/>
-</div>
+<CategoryCard
+  key={index}
+  className="custom-shadow"
+  name={category.name}
+  image={category.image}
+  path={category.path}
+/>

Please verify that this change doesn't break the shadow effect. If CategoryCard doesn't accept a className prop, you'll need to modify it to support this.

Committable suggestion was skipped due to low confidence.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take a look of code rabbit review and fix it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codervivek5, glad to see that you're addressing the feedback. Please let me know if you need any further assistance.

🐰😊

))}
</div>
</div>
Expand All @@ -248,6 +264,13 @@ const Home = () => {
</div>
</div>
</section>







{/* App Download Section */}
<section
className="py-8 sm:py-12 md:py-16 relative mb-[-1px]"
Expand Down Expand Up @@ -286,20 +309,19 @@ const Home = () => {
Subscribe to our newsletter to receive exclusive updates,
promotions, and tips.
</p>
<form>
<form onSubmit={handleSubscribe}>
<input
type="email"
placeholder="Enter your email address"
value={email}
onChange={(e) => setEmail(e.target.value)}
style={{
color: "black",
width: "100%",
borderRadius: "10px",
padding: "10px",
marginBottom: "10px",
border: "1px solid #ccc",
"@media (maxWidth: 780px)": {
width: "80%",
},
}}
/>
<button
Expand All @@ -313,8 +335,13 @@ const Home = () => {
cursor: "pointer",
}}>
Subscribe
</button>{" "}
</button>
</form>
{isSubscribed && (
<div className="popup-message">
<p>Subscribed successfully!</p>
</div>
)}
</div>
</section>
</div>
Expand Down