Unnumbered lists of constructors for polyvariants and extensible variants #987
Conversation
For consistency with variants. In the HTML backend, this merges two <code> elements into one.
The previous rules were applicable only to records and ordinary variants.
There was a problem hiding this comment.
I like the commit that makes the html for variants, polymorphic variants and extension constructor match.
However, I don't think the css changes makes it better.
Also, #971 has other improvements (e.g. a link to the ocamlary module from the index page).
Would you mind if I cherry-pick your commit to #971 and continue from there?
| @@ -534,19 +534,19 @@ div.odoc-spec,.odoc-include { | |||
| margin-bottom: 2em; | |||
| } | |||
|
|
|||
| .spec.type .variant p, .spec.type .record p { | |||
| .spec.type > ol > li p { | |||
There was a problem hiding this comment.
I'm not a fan of these "non semantic" selectors: I think they are more fragile than the previous one.
In my opinion it is better to add the variant class to extension constructors and polymorphic variants.
There was a problem hiding this comment.
I don't see how this is not semantic CSS.
It does look more "rigid" , tied to the specific shape of the markup -- but most things we want to do in CSS require that, and it's sensible to have a li selector when disabling list decorations.
Adding specific cases is fragile and would break if eg. effect declarations with lists of operations were added. IIRC the .variant classes were meant for anchors (links to definitions) and there is nothing to link to for polyvariants, they're structural.
There was a problem hiding this comment.
I don't see how this is not semantic CSS.
I meant that I think it is easier to understand "variants and records in specification of types are shown as ..." rather than "a direct ol child of li that is a direct child of ol are shown as ..."
However, I do agree with:
It does look more "rigid" , tied to the specific shape of the markup -- but most things we want to do in CSS require that, and it's sensible to have a li selector when disabling list decorations.
so maybe a good rule could be
| .spec.type > ol > li p { | |
| .spec.type > li.variant p, .spec.type > li.record p, { |
Adding specific cases is fragile and would break if eg. effect declarations with lists of operations were added.
If effect declarations with list of declarations are added, we need to decide whether we want to show them exactly as we show variants, or differently, and add classes/rules accordingly. So we could say equivalently that .spec.type > ol > li would break if we want those effects to be displayed differently.
IIRC the .variant classes were meant for anchors (links to definitions) and there is nothing to link to for polyvariants, they're structural.
If I understand correctly what you say, no, the .variant classes were not meant for links to definition. Ids are used for that.
There was a problem hiding this comment.
I too prefer to have a more specific CSS rather than a too general one. I believe it helps maintenance by keeping the rules for various parts of the page separate.
The day a list is added to the documentation, we'll have a close look at how it renders and we'll tweak the CSS accordingly.
I also agree that it's not nice to set properties specific to li in a rule that is not restricted to li. I suggest keeping the previous rules as they were except for list-style, which would move into a new more constrained rule:
.spec.type li.variant, .spec.type li.record {
list-style: none;
}
|
@panglesd feel free to cherry-pick changes to your PR |
Fixes #970.
I got too deep into the rabbit hole to notice that #971 exists. My approach preserves the current looks and makes the code for variants and polymorphic variants more consistent.