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

Stop using <details> #92

Closed
trefis opened this issue Oct 24, 2017 · 32 comments
Closed

Stop using <details> #92

trefis opened this issue Oct 24, 2017 · 32 comments

Comments

@trefis
Copy link
Contributor

trefis commented Oct 24, 2017

Which are currently used for includes.

As @dbuenzli pointed out in the past, <details> are terrible to link into and to search a page. We should just avoid them (and use JS instead? :( )

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 24, 2017

Problem with JavaScript is that it doesn't work (unless maybe inlined ?) through the file:// protocol which is the main way of consuming the docsets via odig. Note that if the details are open by default linking works though. Also I suspect you won't be able to recover page search via JavaScript either (unless you hijack in page search which should not be done).

@trefis
Copy link
Contributor Author

trefis commented Oct 24, 2017

Also I suspect you won't be able to recover page search via JavaScript either (unless you hijack in page search which should not be done).

Fair point.
The world really is a sad place, isn't it?

@dbuenzli
Copy link
Contributor

😭

@dbuenzli
Copy link
Contributor

That said I was never really convinced about this kind of folding. I think linking to another page is fine (with the ability to inline via an attribute annotation if wanted).

@leviroth
Copy link

leviroth commented Nov 4, 2017

Open-by-default would be really nice for searchability.

@aantron
Copy link
Contributor

aantron commented Jan 9, 2018

This would also be a workaround for this Safari bug, an interaction between <details> and rem.

aantron added a commit to ocamllabs/odoc-dev that referenced this issue Jan 9, 2018
See

  https://bugs.webkit.org/show_bug.cgi?id=173876

Safari doesn't correctly render elements styled in rem units, when they
follow <details> elements. Probably the right way to deal with this is
to stop emitting <details> elements. However, since we will also change
the stylesheet, this commit changes the CSS for now.

See also ocaml/odoc#92.
@yawaramin
Copy link
Contributor

Hi, I strongly recommend either not using details or opening them by default. Linking to items inside closed details tags is just completely broken and confusing.

E.g., if I link to https://ocaml.janestreet.com/ocaml-core/v0.9/doc/base/Base/List/index.html#val-fold_until the page that loads has no mention of the function. If I don't know about the details gotcha, I have no idea why. If I do, I still have to hunt through expanding all of the closed sections to find the function. On the slim chance that I know about details behaviour and I know exactly which module the function is in, I still have to open it manually.

In short, in-page links should be honoured. That doesn't happen with closed details tags. They need to be open by default.

@Armael
Copy link
Member

Armael commented Feb 27, 2018

Were you considering loading the contents of detailsusing JS? This sounds like a bad idea indeed.
What about having the contents of currents details inlined in the page, and using javascript simply to fold them (by default or not)? If I understood correctly this would work for loading the pages without JS.

@aantron
Copy link
Contributor

aantron commented Feb 27, 2018

I think we will move away from folding completely. We should probably either always inline, with the option of adding a tag or attribute in the .mli file to replace inlining of that specific module by a link to another page, or always link to another page, with the option of inlining it (as suggested by @dbuenzli).

@trefis
Copy link
Contributor Author

trefis commented Feb 27, 2018

Hi, I strongly recommend either not using details or opening them by default.

FTR. they are opened by default, and there is an odoc cmdline flag to change the default. On top of that there is a variety of tags @inline, @opened, @closed one can use override the default on a case by case basis.
Jane Street just made the choice of having folding on by default and opening/inlining selectively.

@lpw25
Copy link
Contributor

lpw25 commented Feb 27, 2018

Personally, I'm in favor of keeping folding but moving away from <details>. It is perfectly possible to solve the internal links issue using JavaScript.

It's not clear to me how to make things work without either folding or inlining a include. If you want the include to be a link, then where is it a link to? Consider:

module type S = sig
  module N : sig
    module type T
  end
  module M : T
end

module Impl : sig
  module type T = sig
    type t = int
  end
end

module Foo : sig
   ...
   include S with module M := Impl
   ...
end

type s = Foo.M.t

If you have include S with ... link to the definition of S, then where are you going to link Foo.M.t to?

You would need to have a page specifically for S with module M := Impl, which raises all kinds of questions about what name that page should have and how to make sure it is unique.

So if you can't use a link you must either inline the contents of the include, which is fine but can quickly become a pain when every module starts with:

include Sexpable ...
include Comparable...
include Hashable...
include With_set...
include With_map...

or you use folding.

@lpw25
Copy link
Contributor

lpw25 commented Feb 27, 2018

Actually, I'd like to adjust my above example from an example of why you can't use links for include to an example of why we'd need to completely reimplement how include was handled to use links for them. You could treat include like functor application, which would allow the Foo.M.t above to link to Impl.T.t. The point above is that you cannot use a link to represent the result of an include statement. However, you can still make links to the elements from the include go to somewhere meaningful.

However, I'm still be very keen to keep support for folding. For certain interfaces it gives a superior browsing experience, allowing you to see all elements of a module when necessary whilst still hiding repetitive details when not needed. I don't mind it not being the default, I would just like to provide the option for people.

@yawaramin
Copy link
Contributor

yawaramin commented Feb 27, 2018

@lpw25

... allowing you to see all elements of a module when necessary whilst still hiding repetitive details when not needed.

Nowadays this is usually done with a TOC panel on the left or right side of the page, see e.g. https://hexdocs.pm/elixir/List.html#delete/2

Folding is IMHO not a good solution because as was noted, it does not honour in-page links. When I give someone a direct link to a function, I want them to be able to jump to it straightaway, without having to open folds.

Btw, I'm not saying get rid of all folds, just saying fold in such a way that we honour in-page links. E.g. we can fold any sections which we know don't contain anchors, like the body of a doc comment.

@lpw25
Copy link
Contributor

lpw25 commented Feb 27, 2018

You can make folding respect in-page links with a little JavaScript and some more precise names for ids. The issue that can't be solved this way is "find in page", which is Daniel's main concern with them. Personally, I think that trade-off is worth making for some APIs, and I would like to have a proper search widget for finding items on the page at some point anyway.

@yawaramin
Copy link
Contributor

Note that it's a trade-off only if you don't have a TOC panel on the left. If you do, you get the best of both worlds–fast overview of the entire module, and no need for folds hence both in-page links and search work.

@lpw25
Copy link
Contributor

lpw25 commented Feb 27, 2018

I don't really see how a TOC helps much. They tend to either contain all elements of a module, and are thus so long as to be fairly useless, like:

https://doc.rust-lang.org/std/primitive.u128.html

or they only contain top-level user specified sections, which is useful but doesn't help much with browsing the modules contents, like:

https://hackage.haskell.org/package/base-4.10.1.0/docs/Control-Monad.html

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

Any new thoughts about this?

If we do remove details I think we should not inline included modules by default. Looking at libraries like Base this would significantly damage the navigation experience as many modules implement well-known interfaces (Comparable, Monad, etc) that most people will want to skip anyway.

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 8, 2018

If we do remove details I think we should not inline included modules by default

You mean including module types right ? Not module implementations.

Base this would significantly damage the navigation experience as many modules implement well-known interfaces (Comparable, Monad, etc) that most people will want to skip anyway.

I wouldn't be so sure about this. I remember being highly irritated by OO doc generators where you always had to look into a class and then all the upper classes. If you have everything on a page you know anything you might use will be here and this knowledge combined with on-page search allows for efficient access and queries.

@trefis
Copy link
Contributor Author

trefis commented Oct 8, 2018

In the absence of any satisfactory replacement for <details> I don't think any action needs to be taken here, at least not right now.
<details> are opened by default, which seems to be the right default.
There are cmdline flags to change the default (which IIRC let you chose between: open, closed, inlined. If that's not the case we should fix that), as well as annotations that can be put in the docstrings to select on each include whether it should be opened, closed, or inlined.

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

Yes, I meant module types, thanks for correcting.

In my opinion, it happens in OO because not many interfaces are as "generic" and "fundamental" as in OCaml. I would say that most people who use Core/Base know what constitutes a "Stringable", "Comparable" or "Hashable" type. Seeing all their items is just distracting (to me).

A counterargument is that code completion in editors, utop and tools like ocp-browser typically include all items inline. We could choose to be consistent with them.

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

@trefis currently the odoc html command accepts a --closed-details option with the description "If this flag is passed <details> tags (used for includes) will be closed by default". I don't think the inlined version is implemented.

This seems related to #160.

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 8, 2018

I would say that most people who use Core/Base know what constitutes a "Stringable", "Comparable" or "Hashable" type. Seeing all their items is just distracting (to me).

Is it really ? I think it's actually wrong to have these as generic documentation bits and they are absolutely not a distraction (to me). To take your first two example interfaces I would certainly like to know at some point which syntax is parsed/emited and how the value is compared exactly.

@trefis
Copy link
Contributor Author

trefis commented Oct 8, 2018

@rizo: thanks for the clarification. Adding an --inline-details might be good then (and perhaps setting it as the default, although I still think that'd be the wrong move at the moment).

Again, I don't think anything else needs to be done now.

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 8, 2018

(And for example when I include Set.S in one of my modules, I don't mind having it inline, if it saves me a trip to the Set module).

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

@dbuenzli I understand your point of view and in a way agree with it. But the problem you describe seems to be orthogonal to the question of whether includes should or should not be inlined.

Allow me to clarify. When a generic module is inlined (as you are suggesting should happen) the generic description will be inlined too, for an abstract type (some S.t). By its nature this documentation will be abstract and probably not very useful to answer your questions like "which syntax is parsed/emited and how the value is compared exactly". So if someone is implementing S, they should probably redeclare the values for which they want to write a specialized, useful documentation. Does that make sense?

I'm aware this is not contributing to the decision for this issue and agree with @trefis that the current situation is acceptable.

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 8, 2018

So if someone is implementing S, they should probably redeclare the values for which they want to write a specialized, useful documentation. Does that make sense?

Yes but IIRC you can have both: include S followed by redefinition of the doc string of some of the doc strings of S. In that case inlining with a potential override taking over what was inlined makes much more sense than not inlining.

And again the doc page acting as a real index of what is in accessible from the module is a useful property in my opinion.

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

Yes but IIRC you can have both

You can and that's what I was trying to describe :). But my conclusion is different, because both the included value and the overridden one would be accessible, it's better to not have the original one inlined.

For example the following:

module type X = sig
  val hello : unit -> string
  (** Generic hello. *)
end

include X

val hello : unit -> string
(** My better hello. *)

Is rendered as:
screen shot 2018-10-08 at 17 18 51

Since the included X is a <details> tag it can be conveniently collapsed. Referencing the overridden hello is currently impossible by the way, but that's a different issue.

@trefis
Copy link
Contributor Author

trefis commented Oct 8, 2018

Two remarks:

  • shadowed items should be present in the signature, but that's just another illustration that our handling of includes is wrong. The backend should be fixed.
  • I don't think shadowing something to provide better documentation is a good workflow that I'd want to push people towards.

@rizo
Copy link
Collaborator

rizo commented Oct 8, 2018

shadowed items should be present in the signature

I'm not sure I understand this, isn't the shadowed item included in the signature? Or did you mean "shouldn't"?

I don't think shadowing something to provide better documentation is a good workflow that I'd want to push people towards.

I personally don't see any issues with this. The included generic functions (regardless of inlining) will never be able to describe exactly how it works for a concrete type (but having it is still useful). In fact functions with default implementation can be redefined for efficiency.

@trefis
Copy link
Contributor Author

trefis commented Oct 8, 2018

I'm not sure I understand this, isn't the shadowed item included in the signature? Or did you mean "shouldn't"?

My bad, I forgot a word, I indeed meant "shouldn't".

@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
@github-actions github-actions bot closed this as completed May 9, 2020
@lpw25
Copy link
Contributor

lpw25 commented Mar 11, 2022

The crappy behaviour of <details> has been fixed in recent Chromes. See "Auto-expand Details Elements" in the Chrome 97 release announcement.

The sensible behaviour also seems to have been added to the HTML spec, although I don't know whether or not that makes it likely to be adopted in the other browsers.

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

No branches or pull requests

8 participants