Skip to content

Conversation

frano-m
Copy link
Contributor

@frano-m frano-m commented Jul 11, 2025

Closes #636.

This pull request introduces updates to the Hero and SectionHero components to support dynamic width calculations alongside height. It also modifies the Section component to provide both height and width dimensions via the useResizeObserver hook. These changes improve layout flexibility and responsiveness.

Updates to Hero and SectionHero components:

  • Added a width prop to the Hero component and updated its default value to 0. The width prop is now used in the <rect> element instead of the hardcoded "100%". (app/components/Home/components/Section/components/SectionHero/components/Hero/hero.tsx: [1] [2]; app/components/Layout/components/AppLayout/components/Section/components/SectionHero/components/Hero/hero.tsx: [3] [4]
  • Updated the SectionHero component to pass the width prop to the Hero component, enabling dynamic width calculation. (app/components/Home/components/Section/components/SectionHero/sectionHero.tsx: [1]; app/components/Layout/components/AppLayout/components/Section/components/SectionHero/sectionHero.tsx: [2]

Enhancements to Section component:

  • Modified the Section component to use the getBorderBoxSize function instead of getBorderBoxSizeHeight, allowing it to calculate both height and width. Updated the children function signature to accept width alongside height. (app/components/common/Section/section.tsx: app/components/common/Section/section.tsxL2-R19)

Styling adjustment:

image

@frano-m frano-m force-pushed the fran/636-hero-svg branch from 6a2aac3 to dcd5254 Compare July 11, 2025 08:05
@frano-m frano-m moved this to Ready for Review in BRC development tasks Jul 11, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR restores the hero background on multiple pages by introducing dynamic width support alongside height in the Section, SectionHero, and Hero components, and ensuring the SVG scales correctly.

  • Section now observes both height and width via getBorderBoxSize
  • SectionHero passes the new width prop down to Hero
  • Hero accepts width and applies it to the <rect>, with styling updated for responsive scaling

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
app/components/common/Section/section.tsx Switched to getBorderBoxSize and updated children signature to (height, width)
app/components/Layout/components/AppLayout/components/Section/components/SectionHero/sectionHero.tsx Passed width into Hero in layout section
app/components/Home/components/Section/components/SectionHero/sectionHero.tsx Passed width into Hero in home section
app/components/Layout/components/AppLayout/components/Section/components/SectionHero/components/Hero/hero.tsx Added width prop to Hero and used it for <rect> width
app/components/Home/components/Section/components/SectionHero/components/Hero/hero.tsx Added width prop to Hero and used it for <rect> width
app/components/Layout/components/AppLayout/components/Section/components/SectionHero/components/Hero/hero.styles.ts Changed SVG to width: 100vw for responsive layout

className?: string;
}

export const Section = ({ children, className }: SectionProps): JSX.Element => {
const sectionRef = useRef<HTMLElement>(null);
const { height } =
useResizeObserver(sectionRef, getBorderBoxSizeHeight) || {};
const { height, width } =
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

The width from useResizeObserver may be undefined on initial render, causing downstream components to receive width=undefined. Consider providing a fallback (e.g., 0) or conditional rendering until both dimensions are available.

Suggested change
const { height, width } =
const { height = 0, width = 0 } =

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do this in the SVG component.

}

export const Hero = ({
gridSize = GRID_SIZE,
height = gridSize * 1.5,
width = 0,
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

Defaulting width to 0 means the <rect> has zero width when no width is passed, making the background invisible. Consider defaulting to the container width or making width required.

Suggested change
width = 0,
width = gridSize,

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with this. Better to show nothing (while waiting for width), than a partial <rect/>.

@@ -2,5 +2,6 @@ import styled from "@emotion/styled";

export const SVG = styled.svg`
position: absolute;
width: 100vw;
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] Hardcoding width: 100vw can lead to misalignment or overflow relative to the container. Consider using width: 100% or binding the SVG’s width to the observed width prop for more accurate scaling.

Suggested change
width: 100vw;
width: 100%;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

@NoopDog NoopDog merged commit 4ed95cc into main Jul 11, 2025
3 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in BRC development tasks Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Restore Hero Background Images on Home, About, and Roadmap Pages
2 participants