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

Less gaps in css #1143

Closed
wants to merge 1 commit into from
Closed

Conversation

EmileTrotignon
Copy link
Collaborator

Reduces gaps in css.

Answers issue #1111.

@EmileTrotignon
Copy link
Collaborator Author

Here is a screenshot of an example page :
image

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

We don't really need less gaps in CSS, we need to remove the gap introduced in #999 by the introduction of the grid system, which disabled margin collapse:

the margins of adjacent grid items do not collapse

(From https://www.w3.org/TR/css-grid-1/#item-margins)

The solution you propose changes many margins/paddings (making the result slightly not spaced enough in my opinion, but that's maybe just habits of the previous style) while not solving the original problem! Edit: it does solve it by always having the top margin equal to zero... I don't think that having all elements have a 0 value for margin-top is a valid assumption to make!

We can solve the problem either by going away from the grid system, or modifying the html layout: grouping odoc-preamble and odoc-content in a single element.
I would prefer the second option. It would not affect ocaml.org since they are using the --as-json option which would be unaffected by this change. However, we should be careful to check if the layout change affects odig stylesheet.

@@ -300,7 +300,7 @@ body.odoc {
display: grid;
grid-template-columns: min-content 1fr;
column-gap: 4ex;
row-gap: 2ex;
row-gap: ex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not valid css and likely a leftover?

Suggested change
row-gap: ex;

@EmileTrotignon
Copy link
Collaborator Author

Oh ok, I did not know about margin collapse, I just git blame on the line that set the topmost margin and found it was quite old, so I thought this was not introduced recently.

I am a bit anxious about changing the order of elements, but I certainly do not want to drop grid.
Another way to do would be to have a rule like :

.odoc-content:first-child {
  margin-top:0 !important;
}

@EmileTrotignon
Copy link
Collaborator Author

I am also not i agree that assuming every element not to have a top-margin is a bad idea, it seems quite reasonable to me, its a convention that works well, and if a margin-top slips in its a very easy patch to remove it.

@panglesd
Copy link
Collaborator

I does not seem acceptable to me to remove all top margins. This would mean that the space between two element would only depend on the top one. That is quite awful!

I opened #1146 with my alternative suggestion.

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.

2 participants