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

Footer Alignment and Spacing #472

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Footer Alignment and Spacing #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions frontend/src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import Footer from '../src/components/Shared/Footer';
import { Outlet } from 'react-router-dom';
import BackToTopButton from './components/Shared/BackToTopButton';
import Preloader from './components/Preloader';
import Metadata from './components/Metadata';
// import Metadata from './components/Metadata';


function App() {
return (
<>
<Metadata />
{/* <Metadata /> */}
<Preloader />
<BackToTopButton />
<Navbar />
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/components/Shared/footer/Content.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ const Section2 = () => {
>
BoardGame Cafe{!isWide && <br />}
</h1>
<p className={`flex ${isWide ? `text-[center] mt-3 ml-[620px]` : `text-base mt-1`}`}>
<p className={`flex ${isWide ? `text-[center] mt-3 ml-[620px]` : `text-base mt-1`}`}>
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

Improve responsive design for copyright text.

The current implementation has several issues:

  1. Using a fixed ml-[620px] margin could cause layout issues on different screen sizes
  2. text-[center] is incorrect (should be text-center)

Consider this more responsive approach:

- <p className={`flex ${isWide ? `text-[center] mt-3  ml-[620px]` : `text-base mt-1`}`}>
+ <p className={`flex ${isWide ? `text-center mt-3 ml-auto` : `text-base mt-1`}`}>
📝 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
<p className={`flex ${isWide ? `text-[center] mt-3 ml-[620px]` : `text-base mt-1`}`}>
<p className={`flex ${isWide ? `text-center mt-3 ml-auto` : `text-base mt-1`}`}>

©2024 by Sip & Play
</p>
</div>
Expand Down Expand Up @@ -167,8 +167,8 @@ const Nav = () => {
const emailAddress = '[email protected]';

return (
<div className="flex md:flex-row flex-col shrink-0 gap-4 sm:gap-20 mx-[40px] md:ml-[30px] md:mr-0">
<div className="flex justify-between md:gap-20">
<div className="flex md:flex-row flex-col shrink-0 gap-4 sm:gap-20 mx-[40px] mb-10 md:ml-[50px] md:mr-0">
<div className="flex justify-between md:gap-28 ">
<div className="flex flex-col gap-2 text-black dark:text-white">
<h3 className="mb-2 uppercase text-black dark:text-white cursor-pointer">About</h3>
{navLinks.map((item, index) => (
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/Shared/footer/Footer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import Content from './Content';
const Footer = () => {
return (
<footer
className="relative h-[800px]"
className="h-[800px]"
style={{ clipPath: 'polygon(0% 0, 100% 0%, 100% 100%, 0 100%)' }}
Comment on lines +6 to 7
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

Footer layout uses complex viewport calculations that may cause inconsistencies

The footer implementation relies on multiple viewport-dependent calculations and positioning that could lead to layout issues:

  • Fixed footer height of 800px combined with calc(100vh+800px) for inner div
  • Sticky positioning with complex calculation calc(130vh-600px)
  • Height transitions from 60px to 1px at md breakpoint
  • Negative top positioning -top-[100vh]

These viewport-dependent calculations and magic numbers make the footer's behavior unpredictable across different screen sizes. Consider:

  • Using simpler, more maintainable height values
  • Replacing viewport-based calculations with fixed or relative units
  • Implementing a more straightforward sticky footer pattern
  • Testing thoroughly on various screen sizes if keeping current implementation
🔗 Analysis chain

Verify footer responsiveness across different screen sizes.

The footer uses a fixed height of 800px with clip-path styling. While this works, please ensure it behaves correctly across different viewport sizes and doesn't cause content overflow issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any viewport-specific styles or media queries related to the footer
rg -l "h-\[(.*?)vh" frontend/src/components/Shared/footer/
rg -l "@media" frontend/src/components/Shared/footer/

Length of output: 163

Script:

#!/bin/bash
# Let's examine the actual content of Footer.jsx to see all height-related classes and responsive styles
rg -A 5 -B 5 "h-\[" frontend/src/components/Shared/footer/Footer.jsx

# Also check for any Tailwind responsive prefixes (sm:, md:, lg:, etc.)
rg "(sm:|md:|lg:|xl:)h-" frontend/src/components/Shared/footer/Footer.jsx

# Look for any sticky positioning or viewport-related calculations
rg "calc\(|sticky|fixed|absolute" frontend/src/components/Shared/footer/Footer.jsx

Length of output: 947

>
<div className="relative h-[calc(100vh+800px)] -top-[100vh] bg-amber-100 dark:bg-black mb-4 pb-10 sm:pb-16">
<div className="h-[700px] md:h-[650px] sticky top-[calc(100vh-600px)]">
<div className="h-[60px] md:h-[1px] sticky top-[calc(130vh-600px)]">
<Content />
</div>
</div>
Comment on lines 9 to 13
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

Consider simplifying the complex positioning logic.

The current implementation uses:

  • Viewport-based calculations (130vh, 100vh)
  • Magic numbers (600px)
  • Different heights for mobile/desktop (60px/1px)
  • Negative positioning (-top-[100vh])

This makes the component brittle and hard to maintain. Consider refactoring to use more standard layout approaches.

-      <div className="relative h-[calc(100vh+800px)] -top-[100vh] bg-amber-100 dark:bg-black mb-4 pb-10 sm:pb-16">
-        <div className="h-[60px] md:h-[1px] sticky top-[calc(130vh-600px)]">
+      <div className="bg-amber-100 dark:bg-black mb-4 pb-10 sm:pb-16">
+        <div className="sticky bottom-0">
           <Content />
         </div>
       </div>

Consider using Flexbox or Grid for layout:

// Alternative approach using flex
<footer className="min-h-screen flex flex-col">
  <div className="flex-grow bg-amber-100 dark:bg-black">
    <div className="sticky bottom-0">
      <Content />
    </div>
  </div>
</footer>

Expand Down
4 changes: 2 additions & 2 deletions frontend/src/router/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import AdminLogin from '../components/Pages/Admin/AdminLogin';
import AdminSignup from '../components/Pages/Admin/AdminSignup';
import ProtectedRoute from './ProtectedRoute';
import Profile from '../components/Pages/Dashboard';
import HelpAndSupport from '../components/Pages/HelpAndSupport';
// import HelpAndSupport from '../components/Pages/HelpAndSupport';
import Contributors from '../components/Contributors';

const router = createBrowserRouter(
Expand Down Expand Up @@ -59,7 +59,7 @@ const router = createBrowserRouter(
<Route path="/membership" element={<Membership />} />
<Route path="/dashboard" element={<Profile />} />

<Route path="/help" element={<HelpAndSupport />} />
{/* <Route path="/help" element={<HelpAndSupport />} /> */}
<Route path="/contributors" element={<Contributors />} />
</Route>
)
Expand Down
Loading