sambecker / exif-photo-blog

Photo blog, reporting 🤓 EXIF camera details (aperture, shutter speed, ISO) for each image.
https://photos.sambecker.com
711 stars 117 forks source link

Centering content with env variable #130

Closed Johnomated closed 1 month ago

Johnomated commented 1 month ago

Issue #65 talked about centering content on the page. It was mentioned that the layout is deliberately left-aligned, and it is the intended design. That is fine and my intention isn't to question the design choice, but on desktop with a higher resolution monitor over half the page is empty space.

I created an environment variable, NEXT_PUBLIC_CENTER_CONTENT, and a CENTER_CONTENT variable in config.ts which returns true if process.env.NEXT_PUBLIC_CENTER_CONTENT === '1', otherwise it is false if not '1' or the env variable doesn't exist. I imported CENTER_CONTENT in layout.tsx and added a few lines of tailwind for lg: width or above to center the content if CENTER_CONTENT is true. If CENTER_CONTENT is false, then it defaults to the current layout. The layout shouldn't change on smaller screens/mobile even if CENTER_CONTENT is true.

Is this something you would consider merging? If so, do you want to keep it as true/false centered or left-aligned, or also have a way for right alignment? I can make a pull request with my changes if this is wanted as a feature and edit it as requested.

centered_grid centered_frame

sambecker commented 1 month ago

Hi @Johnomated definitely something I would consider.

Think there's some nuance around the breakpoints, and how the site transitions gracefully from left-aligned to centered. Additionally, it might be nice for the main content to be perfectly centered, while the sidebar has a negative offset. If it all worked perfectly, it could possibly be the default setup.

I'm open to seeing what you come up with. If it remains a configuration, I wonder if we call it NEXT_PUBLIC_CENTERED_ON_LARGE_SCREENS?

Johnomated commented 1 month ago

Hey @sambecker, I had a chance to look into it more tonight. I forked the repo and deployed it here.

Edit: Here's a version where the main content and navbar/footer should always be centered

I ended up reverting the main layout file back to how it was originally and instead modified the SiteGrid component file. Initially I tried to change the use of grid to flex to get the main content centered but ran into a lot of issues with the PhotoLarge and PhotoDetailPage views. SiteGrid wasn't happy that I tried to turn it into SiteFlex so I scrapped that for now.

I messed with different grid and column setups and the simplest solution I came up with, without needing to potentially change a lot, was using mx-auto then starting the main content in the second grid spot and making the sidebar span two columns instead of three. The problem with this method is the main content isn't perfectly centered, which I agree would be nice. I think it looks good on pages with the sidebar, but you can tell it's slightly off center on other pages like shot-on. I can look more into that if you'd like. I did try some column setups where the main content was perfectly centered but either ran into issues with the content looking too small since it had to fit in less columns, or the sidebar having breakpoint issues because it was so far to the right.

You were correct about the breakpoint nuances. There was an awkward gap on the left side when using lg, so I ended up using xl instead for the breakpoints. Once the breakpoint is hit the main content goes back to column one and the sidebar is switched back to three columns. I changed the env variable to the name you suggested, NEXT_PUBLIC_CENTERED_ON_LARGE_SCREENS, but the naming of that if you want it to stay as a configuration is totally up to you.

sambecker commented 1 month ago

Hi @Johnomated thanks so much for putting this together. Will take a look soon. Apologies for the delay.

Johnomated commented 1 month ago

@sambecker it's no problem and no rush! Let me know if you want anything changed or have any better ideas whenever you get a chance to look.

sambecker commented 1 month ago

@Johnomated experimenting with default centering on large screens—based on main content width, ignoring sidebar width—in #135 (after bringing your commits over)

Can be previewed here—LMK what you think

Johnomated commented 1 month ago

@sambecker Looks good to me! I agree with centering it by default on large screens, and I think it works better when it snaps back to the left side instead of staying centered on smaller screens like it was in my preview. Can't think of anything I would change off the top of my head.

sambecker commented 1 month ago

Awesome! Just merged cc @geczy re:original request for centered layouts

Geczy commented 1 month ago

amazing, thanks so much !