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

Fix missing padding on .container. #55

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Conversation

Leftium
Copy link

@Leftium Leftium commented Jan 29, 2025

Fixes picocss#654.

@Yohn Yohn merged commit f7c22db into Yohn:main Feb 5, 2025
@Yohn
Copy link
Owner

Yohn commented Feb 5, 2025

Sorry for the late response, I'm finally starting to get over this flu so I'm back!

@Yohn
Copy link
Owner

Yohn commented Feb 6, 2025

@Leftium Any reason why we don't put padding on the .container from the small breakpoint or smaller? And this also removed the padding on the .container-fluid.

I wrote out below after investigating this some more, but will be update the padding here shortly on these containers.


This is my theory as to why it was like this previously

Reasoning

Before your change:

The .container-fluid always has the padding on the sides.
The .container only had padding-inline when below the smallest viewport.
The .container would then not have padding-inline when its above the breakpoints and viewports.

After your change:

The .container-fluid doesnt have any padding now. (Yes this can be fix but adding the padding for the fluid class.)
The .container only has padding when the view is larger than the smallest viewport only.

Intent

How I see this being intentional is because when the screen width is less than the smallest breakpoint, we would want padding on the .container because it is up against the edge of the screen.
Since the breakpoints center the .container it is not on the edge of the screen, therefor doesn't need padding.

@Leftium
Copy link
Author

Leftium commented Feb 6, 2025

@Leftium Any reason why we don't put padding on the .container from the small breakpoint or smaller? And this also removed the padding on the .container-fluid.

@Yohn I think your theory is correct! I forgot the padding would still apply if the viewport width is less than width of the smallest breakpoint. (And .container-fluid)

I think we should revert this PR.


Here is the reason I wanted padding: 0 for the smallest breakpoint and non-zero padding for the other breakpoints; perhaps it is a different bug or I need to use Pico CSS differently:

  • When the body's background color is different than the .container, all the contents of .container kiss the edge of the body background color. It looks better if there is space (padding) between the contents and body background.
  • It looks particularly bad with cards (<article>) due to the border and box-shadows appearing "clipped" on the sides.
  • At the smallest breakpoint and below, the body's background is no longer visible. So I thought there was no longer a need to separate the contents from the background (but I was wrong: there is still a need for space between the content and edge of the screen).

Here .container has a white background while body's is gray. Without padding on .container, the full-width buttons kiss the gray background:

image

At the smallest breakpoint, the gray body background is no longer visible. (But I guess padding is still needed!):

image

@Leftium
Copy link
Author

Leftium commented Feb 6, 2025

I edited my previous comment several times as I figured things out in real-time, so make you read the latest version!

On a related note, the way Pico CSS adds "mandatory" margin (the body background color gray region in the screenshot above) made it difficult to control how the page is printed. (The printer also wanted to add its own margin.)

After I figured out what the issue was, I solved it with this bit of CSS: https://github.com/Leftium/leftium.com/blob/2da29794575ef604bafec8cdf8fe5aa551a5efdf/src/app.scss#L30-L34

@Leftium
Copy link
Author

Leftium commented Feb 6, 2025

Here's a suggestion for how to resolve this:

  • There is always padding. For all breakpoints; for any width.
  • Optional: the breakpoints are adjusted to to account for the additional padding.
    • I think just increasing the max-widths would be enough.

Leftium referenced this pull request Feb 11, 2025
…gh space for the arrow, and keep the arrow at the top when theres multi-lines for the sumary text.

Reverted back the container padding.
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.

Default padding on .container is overridden by conditional padding of smallest breakpoint.
2 participants