Skip to content

Refine includes#612

Merged
jonludlam merged 4 commits intoocaml:masterfrom
dbuenzli:refine-includes
Mar 11, 2021
Merged

Refine includes#612
jonludlam merged 4 commits intoocaml:masterfrom
dbuenzli:refine-includes

Conversation

@dbuenzli
Copy link
Copy Markdown
Contributor

This PR is build on top of #607 the relevant commits are the 4 last ones (sorry for always proceeding that way but changing the markup bit-by-bit in different branches is too unconvenient).

The current structure of includes is quite involved and messy. For one non-inlined include we have:

div.odoc-include
  div."spec include"
    div.doc 
      content of doc string (may be multiple paragraphs)
      details 
        summary
          span.def
            code
      include defs

This PR proposes to change the structure to

div.odoc-include 
  div.spec-doc
    content of doc string
  details
    summary."spec include"
      code
    include defs

Except for the docstring of the include coming before the specification (it would have been nice to put in the summary, but the content model disallows it). This more regular with other structure items which have the following markup:

  div.odoc-spec 
    div."spec $(kind)"
      code
    div."spec-doc"
      content of doc string

In particular structure item doc strings are always in a spec-doc div and the spec class is always on elements spanning directly the source of structure items.

I have adapted the odoc.css and checked that stuff like double includes like this one work. I took the oportunity to fix #586 along the way.

/* Module member specification */

.spec:not(.include), .spec.include details summary {
.spec {
Copy link
Copy Markdown
Contributor Author

@dbuenzli dbuenzli Feb 27, 2021

Choose a reason for hiding this comment

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

For people devising markup this is a good example of why classes should not be used for vastly different contexts.

Previous to this commit the spec include classes were used on a div wrapping the whole include and its markup content. But otherwise the spec class was always used only on divs simply wrapping the source of structure items. This entailed this incredibly complex selector for styling structure items sources which, with the new structure, now boils down to a simple .spec.

Copy link
Copy Markdown
Contributor

@Drup Drup left a comment

Choose a reason for hiding this comment

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

The markup for includes was definitely clunky, this is much better.

@dbuenzli It would be super helpful if you could write some quick guidelines on top of the HTML rendered detailing the general philosophy guiding the placement of the classes and the shape of the markup (you explained it in your various PRs, but I think it should be in the code).

Move from:

```
div.odoc-include
 div."spec include"
   div.doc
     content of doc string (may be multiple paragraphs)
     details
       summary
         span.def
           code
       include defs
```

to

```
div.odoc-include
  div.spec-doc
    content of doc string
  details
    summary."spec include"
      code
    include defs
```
@dbuenzli
Copy link
Copy Markdown
Contributor Author

quick guidelines on top of the HTML rendered detailing the general philosophy guiding the placement of the classes and the shape of the markup

Not sure I can establish a general philosophy from gut feelings :-) But I can try to add something at some point another markup PR.

(you explained it in your various PRs, but I think it should be in the code).

Do you have particular comments in mind ?


The PR has been rebased.

@jonludlam
Copy link
Copy Markdown
Member

Thanks @dbuenzli!

@jonludlam jonludlam merged commit b437d92 into ocaml:master Mar 11, 2021
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.

CSS changes lost the 'include' visual cue

3 participants