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

Replace the content class by odoc #298

Closed
dbuenzli opened this issue Feb 11, 2019 · 12 comments · Fixed by #589
Closed

Replace the content class by odoc #298

dbuenzli opened this issue Feb 11, 2019 · 12 comments · Fixed by #589

Comments

@dbuenzli
Copy link
Contributor

Currently the main body in the output is classified as content which is first pretty meaningless and second, quite bad the day you want to compose the outputs and the css with other documents. I suggest this should be odoc.

@rizo
Copy link
Collaborator

rizo commented Feb 11, 2019

First of all, I'll clarify why I decided to add the content class (I understand you're not suggesting to remove it).

  • When working with the sidebar it was important to control the layout of the main content and its offset, having a separate CSS class helps with that.
  • Once we start adding other layout elements, like a search bar, having a clear boundary for the content is going to be even more important.

quite bad the day you want to compose the outputs

I asked this once already I think, but I don't know (or remember) what's the concrete use case for this. In what circumstances would we want to compose multiple documents? Wouldn't it make sense to use article tags in that case anyway?

I suggest this should be odoc.

Hm, unfortunately I find it even more ambiguous than content. The whole document is similarly produced by odoc and within that document this class wouldn't add any semantic meaning at all. In case the content is embedded into another document, I would say that the parent document could add whatever class it finds appropriate for the content (it could be odoc, api, reference, etc.).

Having said that, I think it would be better to replace <div class="content"> by just <main>. This clearly indicates the semantic meaning of the content and helps with accessibility features like reader modes in browsers. It also clearly and uniquely express what is the composable part of the document.

What do you think?

@rizo
Copy link
Collaborator

rizo commented Feb 11, 2019

Just realised that your motivation is probably embedding odoc's main content into odig. In which case my suggestion still applies. I think you'd want to extract the main tag content from the output, wrap it into article and add a custom class like odoc.

On a related note, I also think that having a way to supply a custom document template for odoc would help with this. It was briefly discussed during the fragment PR, if I'm not mistaken. Essentially it would allow to embed odoc's output into existing websites (like ocaml.org).

@dbuenzli
Copy link
Contributor Author

Wouldn't it make sense to use article tags in that case anyway?

You don't care about the tag. You just care about being able to label the subtree that is output by odoc. The most sensitive seems to be to label it by odoc no ?

@rizo
Copy link
Collaborator

rizo commented Feb 11, 2019

You don't care about the tag.

Precisely. It's not odoc's responsibility to decide how its content is embedded into other documents. I (personally) was making a suggestion, given that article seems appropriate for these situation.

You just care about being able to label the subtree that is output by odoc.

I would say odoc shouldn't care about the class in the same way it shouldn't care about the tag (see above). In my understanding the intent to label the subtree comes from the fact that the output generated by odoc is being embedded. Which, to me, suggests that the labeling should be done by the tool doing the embedding.

Of course having something like <main class="odoc"> wouldn't hurt (and I'd be happy to add it), but for standalone odoc documents it is redundant, IMO.

@dbuenzli
Copy link
Contributor Author

Just realised that your motivation is probably embedding odoc's main content into odig

No I absolutely don't do anything like this in odig. But if one day we introduce a mode where only page subtrees are output our CSS rules can compose well with them.

On a related note, I also think that having a way to supply a custom document template for odoc would help with this.

It's more flexible if you simply output subtrees rather than whole page and let templating occur outside the way your templating engine wishes.

I would say odoc shouldn't care about the class in the same way it shouldn't care about the tag (see above). In my understanding the intent to label the subtree comes from the fact that the output generated by odoc is being embedded. Which, to me, suggests that the labeling should be done by the tool doing the embedding.

This misses the point. The point is to be able to easily reuse the CSS and target it specifically to the odoc fragment on the page.

@rizo
Copy link
Collaborator

rizo commented Feb 11, 2019

It's more flexible if you simply output subtrees rather than whole page and let templating occur outside the way your templating engine wishes.

May I ask how it would be more flexible? For example, custom document template could allow both support files and the main content to be injected into the template (not to mention the search bar in the future...). I don't see how that would be done with just content output.

This misses the point. The point is to be able to easily reuse the CSS and target it specifically to the odoc fragment on the page.

Indeed, you're right. In that case we would need to prefix all odoc-specific CSS selectors with .odoc, right?

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Feb 11, 2019

May I ask how it would be more flexible? For example, custom document template could allow both support files and the main content to be injected into the template (not to mention the search bar in the future...)

It's more flexible because you basically declare that this is not odoc's problem. You don't have to commit to any templating framework. You just let the client input the file hierarchy and document fragments it generated and let it do whathever it wishes with it. In other words: there's no inversion of control and odoc doesn't need to care about that (except for the ability to output a single element without the "page chrome").

We already went the document template route once a long time ago. Let's not start this again.

Indeed, you're right. In that case we would need to prefix all odoc-specific CSS selectors with .odoc, right?

Somehow yes. Unless there's an easier way to scope in these beautiful "modern" CSS times.

@dbuenzli
Copy link
Contributor Author

Just to be clear this issue is absolutely not urgent, it's really about forward looking CSS. Though for example in odig I'm now using this somehow dubious content class to get the right page geometry on the package index list and that's the reason why I raised the issue.

@rizo
Copy link
Collaborator

rizo commented Feb 11, 2019

We already went the document template route once a long time ago. Let's not start this again.

Feel free to point me to any previous discussions I should be aware of :)

I'm now using this somehow dubious content class

I wouldn't say it's dubious. Many modern website use this convention including, for example, Wikipedia:

<div id="content" class="mw-body" role="main">...</div>

Regarding scoping: the only issue I see with the .odoc class prefix is that we might have some elements outside of the main tag (which I think should replace the current div.content), so the class might end up being used in multiple places.

I'm curious what @ryyppy thinks about this.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Feb 11, 2019

I wouldn't say it's dubious. Many modern website use this convention including, for example, Wikipedia:

They may be modern but I really don't see the point of such a generic classification and again the only thing it will bring you is clashes with other "content" if you try to compose, because everyone thinks it has the "contents".

Regarding scoping: the only issue I see with the .odoc class prefix is that we might have some elements outside of the main tag (which I think should replace the current div.content), so the class might end up being used in multiple places.

Note that I don't see any main tag being generated in the current output (and I don't think it's a problem...). The only thing I'm saying is that in general whether output as a full page or as a fragment all of odoc's output should be contained by a single root element (whatever tag that is) which should be tagged with an odoc class. I'm not sure it's worth making things more complex than that.

@github-actions
Copy link

github-actions bot commented May 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label May 1, 2020
@dbuenzli dbuenzli removed the stale label May 1, 2020
@github-actions
Copy link

github-actions bot commented Jun 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. The main purpose of this is to keep the issue tracker focused to what is actively being worked on, so that the amount and variety of open yet inactive issues does not overwhelm contributors.

An issue closed as stale is not rejected — further discussion is welcome in its closed state, and it can be resurrected at any time. odoc maintainers regularly check issues that were closed as stale in the past, to see if the time is right to reopen and work on them again. PRs addressing issues closed as stale are as welcome as PRs for open issues. They will be given the same review attention, and any other help.

@github-actions github-actions bot added the stale label Jun 1, 2020
@dbuenzli dbuenzli added not stale and removed stale labels Jun 1, 2020
dbuenzli added a commit to dbuenzli/odoc that referenced this issue Feb 12, 2021
1. Classify the root odoc tree (currently a `body` element) as `odoc`.
   This closes ocaml#298 and will help stylesheet management/composition
   if ocaml#374 ever becomes a reality.
2. Classify toplevel `nav` as `odoc-nav` (new class)
3. Classify module preamble `header` as `odoc-preamble` (new class)
4. Reclassify the toc `nav` from `toc` to `odoc-toc`.
5. Reclassify the content `div` rom `content` to `odoc-content`.

Closes ocaml#298.
dbuenzli added a commit to dbuenzli/odoc that referenced this issue Feb 24, 2021
1. Classify the root odoc tree (currently a `body` element) as `odoc`.
   This closes ocaml#298 and will help stylesheet management/composition
   if ocaml#374 ever becomes a reality.
2. Classify toplevel `nav` as `odoc-nav` (new class)
3. Classify module preamble `header` as `odoc-preamble` (new class)
4. Reclassify the toc `nav` from `toc` to `odoc-toc`.
5. Reclassify the content `div` rom `content` to `odoc-content`.

Closes ocaml#298.
jonludlam pushed a commit that referenced this issue Feb 25, 2021
1. Classify the root odoc tree (currently a `body` element) as `odoc`.
   This closes #298 and will help stylesheet management/composition
   if #374 ever becomes a reality.
2. Classify toplevel `nav` as `odoc-nav` (new class)
3. Classify module preamble `header` as `odoc-preamble` (new class)
4. Reclassify the toc `nav` from `toc` to `odoc-toc`.
5. Reclassify the content `div` rom `content` to `odoc-content`.

Closes #298.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants