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 gaps between preamble and content #1146

Closed
wants to merge 3 commits into from
Closed

Conversation

panglesd
Copy link
Collaborator

Fix #1111 (alternative to #1143).

Since the introduction of the grid system (#999), margin collapse did not work between the preamble and content, creating gaps on which people fell:

the margins of adjacent grid items do not collapse

This PR slightly changes the html layout, putting the odoc-preamble and odoc-content in an odoc-main element, which in my opinion make more sense than previously:
Previously, they were separated by the nav bar. I wonder if this was a deliberate choice, eg for accessibility reasons, or if it just happened to be like that?

This does not affects ocaml.org (which uses the json output), and from my tests it does not seem to affect odig (let's ping its author, @dbuenzli, so he's aware of the potential layout change!).

Since the introduction of the grid system, margin collapse did not work between
the preamble and content as they are not in the same element.

It makes sense to have them in the same element, as they are to be displayed in
a single flow.

Signed-off-by: Paul-Elliot <[email protected]>
@panglesd panglesd mentioned this pull request Jun 10, 2024
Signed-off-by: Paul-Elliot <[email protected]>
@EmileTrotignon
Copy link
Collaborator

EmileTrotignon commented Jun 10, 2024

Nice, the CI needs some work (promoting test ?), but I like the PR.

Signed-off-by: Paul-Elliot <[email protected]>
@dbuenzli
Copy link
Contributor

I don't think this should be done. Basically all the different page elements should be at the same level, you can then easily dispatch them using CSS grid.

@dbuenzli
Copy link
Contributor

For example Here's a design I can no longer do:

|         nav        |
| preamble | content |
| toc      |         | 

Please don't break odoc's markup because your CSS is broken. odig's CSS has no problem with the current markup.

@EmileTrotignon
Copy link
Collaborator

For example Here's a design I can no longer do:

|         nav        |
| preamble | content |
| toc      |         | 

Please don't break odoc's markup because your CSS is broken. odig's CSS has no problem with the current markup.

I think this is a good reason not to go forward with this PR, thanks for the feedback.

@dbuenzli
Copy link
Contributor

In general adding more divs to solve CSS layout problem is the wrong way to go.

I would rather suggest someone takes a deep look at rebooting and trimming down odoc.css. You have 1200 lines of CSS. odig has 270 lines of CSS for a generally more readable result. Granted a few new things like tables are not added yet but still, I think you have a problem here.

The thing you absolutely do not want is thousands of lines of CSS which is easy to get when people "fix" the CSS here and there (personally I don't think CSS and web design is really suitable for free software collaboration but that's another story – and the reason why I gave up trying to help with odoc.css here).

@dbuenzli
Copy link
Contributor

And btw. since I'm looking at odig's CSS this PR certainly does break odig's CSS see here. On narrow (mobile) viewports the stylesheet collapses the page structure to:

| nav      |
| preamble |
| toc      |
| content  |

@panglesd
Copy link
Collaborator Author

Thanks for your input.

I would not author odoc's CSS either. I'm just trying to fix some problems without rewriting the whole thing, which may indeed be a bad idea. Until someone (with the skills to do it) rewrite the whole thing, I'll stick with that.

On the topic of this PR: Yes, you are right, I underestimated the distinction between the preamble and the content.
So maybe the problem is more that odoc's stylesheet is not making it clear that the reason their is a gap (as in #1111) is that we are switching from the preamble to the content.

I can try to open a PR with a similar approach to what is done in odig? Or do you want to take care of that @EmileTrotignon ?

image

@panglesd panglesd closed this Jun 10, 2024
@EmileTrotignon
Copy link
Collaborator

@panglesd I have a patch ready, I will open a PR.

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.

Mind the gap
3 participants