Eemeli/enhancement/619 new header component implementation#632
Conversation
Dev & main merge
Tests were failing and therefore need to be removed to not block the builds
Dev main merge
Dev / testing the main ci pipe
…der/Footer structure
…mations and image still show up under the header
Merge Dev To Main
… some tweaking and futureproofing
…9-New-Header-Component-Implementation
…button components
|
Switched base branch, may cause issues. "Pulled latest from main, and updated some styling" Main is not the latest, its our production branch. |
Skoivumaki
left a comment
There was a problem hiding this comment.
Branch is failing to run currently due to: "Module not found: Can't resolve '@/shared/assets/images/mainpage/topimage.png" missing.
By your screenshots the new header looks solid 👍
I noticed you left a lot of comments that really don't add anything or feel like your personal notes. Please remove them to reduce bloat and make code more readable.
When you fix the build error, will review the visual side, but its probably fine 😄
There was a problem hiding this comment.
This is cool, but why was it implemented and was its effects tested all around, since its handling root layout
There was a problem hiding this comment.
LayoutBackgroundController was introduced during the header refactor when the image on the homepage (topimage) was moved into the new Header component. At that stage the image caused visual conflicts with the global background texture rendered by LayoutWithBackground, especially on the main page where the image overlaps the layout background.
The controller was added as a client-side wrapper so that background behavior could be adjusted based on the route without converting RootLayout into a client component. This allowed using client routing logic if needed.
The layout has been tested with this structure and it works correctly across the site. Currently the controller only passes children to LayoutWithBackground, so it does not actively change behavior anymore.
This can be removed if necessary, but it is functioning correctly in this state.
| * LayoutBackgroundController is a client wrapper for LayoutWithBackground. | ||
| * | ||
| * Purpose: | ||
| * - Allows controlling background behavior in a Client Component without converting RootLayout into a client component. |
There was a problem hiding this comment.
Why do we need to control a background image?
There was a problem hiding this comment.
Originally the goal was to allow conditional background rendering depending on the page. The homepage image (topimage) initially caused layout issues with the global background texture, so this was the solution at that point.
After further adjustments the layout now renders correctly with the background enabled everywhere. Because of that, the controller is no longer strictly required and can be removed if needed.
| </head> | ||
| <body> | ||
| <LayoutWithBackground> | ||
| <LayoutBackgroundController lng={lng}> |
There was a problem hiding this comment.
You are passing language parameters to a background controller? Do the tiles change color if in english? xd
There was a problem hiding this comment.
It was originally passed so the controller could detect routes like /${lng} if we needed language aware background logic. This is not currently used and does not do anything, i will remove it as a small cleanup. Not sure why i thought this would be useful, i admit it is a bit odd :D
| * @param imagePath - Path to the background image (defaults to main background) | ||
| * @param alt - Alt text for the background image (for accessibility) | ||
| * @param shouldBeLazyLoaded - Whether to lazy load the background image (not used with CSS background) | ||
| * Responsibilities: |
There was a problem hiding this comment.
Is this documentation too much? IMO good documentation should be kept brief enough to explain what each parameter or prop does. We don't need to tell the developer what the component should NOT be used for. Adding notes for developers is good too, but not when you are repeating what params do.
There was a problem hiding this comment.
Noted, removed unnecessary comments from this file.
| * Notes: | ||
| * - Background visibility is controlled via `showBackground`. | ||
| * - This component is intended to be used at layout level. | ||
| * - Route-based background logic should be handled by a wrapper |
There was a problem hiding this comment.
Is it really route based though when the only route we are using is language? 🤔
There was a problem hiding this comment.
Good point, this is some leftover documentation from the earlier implementation, and will be removed.
There was a problem hiding this comment.
Was fixing feedback button bug part of the issue? @Rutjake
"Implemented responsive styling and layout improvements." This is too broad to explain why this change was done. (we currently have an active bug fix issue, with similar z-index problems. The reason I'm mentioning this is, will this change affect those bug fixes too?)
There was a problem hiding this comment.
The feedback button was rendered below the header in the stacking order when scrolling to the top, thus becoming unresponsive. The only changes i made was in the .SideButton (z-index: 1000 !important;) and the .FeedbackCardContainer (z-index: 1001;).
| padding-top: 1rem; | ||
| justify-content: center; | ||
|
|
||
| /* Footer controls layout now (was previously inside SocialSection component) */ |
There was a problem hiding this comment.
This comment could have worked really well as a commit message instead.
There was a problem hiding this comment.
True. I will try to write more specific commit messages in the future.
| const { className = '', socialIconLinks } = props; | ||
|
|
||
| const params = useParams(); | ||
| const lng = (params?.lng as string) ?? 'fi'; |
There was a problem hiding this comment.
For the future: Page already defaults to finnish if no lng is provided, no need to add failsafes 👍
There was a problem hiding this comment.
You removed WallIntroAnimation from mainpage but still made changes to the component? Feels like you were planning to use it, but for some reason didn't? Always explain your thought process in PR or by commits.
There was a problem hiding this comment.
The changes to WallIntroAnimation were made earlier to fix a DOM race condition and to properly handle timeout cleanup and localStorage usage.
During the header refactor the animation was removed from the MainPage because the new header layout replaced its role. The fixes were kept since the component might still be reused later and they prevent the previous runtime errors.
The intention was also to improve stability and performance, but I forgot to document that in the PR. Thanks for pointing it out 👍
| @@ -1,5 +1,5 @@ | |||
| import type { Meta, StoryObj } from '@storybook/nextjs'; | |||
| import { socialIconLinks } from '../../model/data/socialSectionMenu'; | |||
| import { socialIconLinks } from '../../../../shared/const/socialSectionMenu'; | |||
There was a problem hiding this comment.
Do @/shared/const/socialSectionMenu instead. (more readable and achieves the same thing)
Also apply the fix in other files.
There was a problem hiding this comment.
Noted, fix applied 👍
…s with the path alias (SocialSectionMenu), removed unused lng props from LayoutBackgroundController, Clarified and cleaned up layout related code introduced during the header refactor, removed unnecessary comments, cleaned up the header wrapper from the widgets folder (no header implementation needed here).
Rutjake
left a comment
There was a problem hiding this comment.
Great job! The component looks exactly as it should. However, when you remove components like Button or ExternalLinks, remember to also remove the unused imports. If you’re using VS Code, the ESLint extension is really helpful for catching these.
|
|
||
| return ( | ||
| <div className={cls.MainPage}> | ||
| <WallIntroAnimation renderOnce={true} /> |
There was a problem hiding this comment.
Removing WallIntroAnimation was not part of the issue, but its position in the code needs to be adjusted to ensure it functions as expected. @EemeliJ It should work if you add it below the Header component?
…pletely, should work now with the new header. Also removed some unused imports.
…SessionStorage, also refactored timing logic removing a redundant timeout that set the rendered state too early.
📄 Pull Request Overview
Closes [619]
🔧 Changes Made
✅ Checklist Before Submission
console.log()or other debugging statements are left ✅📝 Additional Information
Provide any additional context or information that reviewers may need to know: