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.

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

Adds a layout for the app route group #42

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

Conversation

golddydev
Copy link
Collaborator

Make app layout (with responsiveness)

  • main header
  • sidebar

@golddydev golddydev requested review from schaier-io and mrgrauel and removed request for schaier-io March 6, 2025 17:23
@golddydev
Copy link
Collaborator Author

Desktop

image

Copy link
Collaborator

@mrgrauel mrgrauel left a comment

Choose a reason for hiding this comment

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

I receive the following warnings in the console. I would highly recommend that we handle warnings as errors in the development process.

hook.js:608 Image with src "/logos/sokosumi-logo-black.svg" was detected as the Largest Contentful Paint (LCP). Please add the "priority" property if this image is above the fold.
Read more: https://nextjs.org/docs/api-reference/next/image#priority
dashboard:1 The resource http://localhost:3000/_next/static/css/app/layout.css?v=1741330991996 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate `as` value and it is preloaded intentionally.

@golddydev
Copy link
Collaborator Author

I receive the following warnings in the console. I would highly recommend that we handle warnings as errors in the development process.

hook.js:608 Image with src "/logos/sokosumi-logo-black.svg" was detected as the Largest Contentful Paint (LCP). Please add the "priority" property if this image is above the fold.
Read more: https://nextjs.org/docs/api-reference/next/image#priority
dashboard:1 The resource http://localhost:3000/_next/static/css/app/layout.css?v=1741330991996 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate `as` value and it is preloaded intentionally.

I am checking this warnings now

@golddydev
Copy link
Collaborator Author

I receive the following warnings in the console. I would highly recommend that we handle warnings as errors in the development process.

hook.js:608 Image with src "/logos/sokosumi-logo-black.svg" was detected as the Largest Contentful Paint (LCP). Please add the "priority" property if this image is above the fold.
Read more: https://nextjs.org/docs/api-reference/next/image#priority
dashboard:1 The resource http://localhost:3000/_next/static/css/app/layout.css?v=1741330991996 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate `as` value and it is preloaded intentionally.

About preload warning: I just fixed. (caused by image src being "#")

About image priority issue: I am going to fix that in another PR which update logo components

@golddydev golddydev requested a review from mrgrauel March 7, 2025 09:36
Copy link
Collaborator

@mrgrauel mrgrauel left a comment

Choose a reason for hiding this comment

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

The logo should be in the sidebar according to the design specs.

image

@golddydev golddydev requested a review from mrgrauel March 7, 2025 15:18
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should componetize this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make this component simple using nav-menu and nav-link

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets also extract the component here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I extracted nav-menu from main-nav and mobile-nav

@golddydev golddydev requested a review from schaier-io March 8, 2025 10:39
@golddydev golddydev self-assigned this Mar 8, 2025
Copy link
Collaborator

@mrgrauel mrgrauel left a comment

Choose a reason for hiding this comment

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

Should we implement a specific mobile design? As I understand it, we are focusing on a responsive design for desktop that should work reasonably well on mobile. However, there should not be an explicit mobile implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend to name it just header.tsx, because we are already in the (app) folder.

Copy link
Collaborator Author

@golddydev golddydev Mar 9, 2025

Choose a reason for hiding this comment

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

The reason why I prefixed app before header and sidebar, is that
we have Sidebar (from shadcn), which makes name conflicts

So that is why I named it as app-sidebar

Well do you prefer the say import Sidebar as ShadcnSidebar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the issue with the Sidebar, that makes sense to name the export it as AppSidebar, but I would rename the folder.

Sorry but naming is for me important... I proposed a change how I think it makes it clear to understand even in 6 months.

Rename AppHeader to Header -> 7734386
Rename app-sidebar folder to sidebar -> 57a2704

What do you think? Any opinion about these changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, then Let's stick to that convention.

Never prefix components folder or exported name with parent folder

Then we can use import as and default export Sidebar instead of AppSidebar

@mrgrauel
Copy link
Collaborator

mrgrauel commented Mar 8, 2025

Also the folder app-sidebar should just be named sidebar, doesn't it? Same idea as with the app-header.tsx renaming to header.tsx.

@golddydev
Copy link
Collaborator Author

golddydev commented Mar 9, 2025

Should we implement a specific mobile design? As I understand it, we are focusing on a responsive design for desktop that should work reasonably well on mobile. However, there should not be an explicit mobile implementation.

I don't implement any specific mobile design.

If you are talking about mobile-nav, that is for responsive.

Main nav is top sticky nav bar,
mobile nav is drawer from right side.

@mrgrauel mrgrauel changed the title Feat/app layout Adds a layout for the app route group Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants