Media in odoc 3! #1184
Media in odoc 3! #1184
Conversation
panglesd
force-pushed
the
media-in-odoc3
branch
from
65fcc81
to
08d6e5d
Compare
doc/odoc_for_authors.mld
Outdated
| video. Each of them can refer to the file either using an asset | ||
| reference, or a direct link. | ||
|
|
||
| The markup is [{<media>:link}], [{<media>!ref}], |
There was a problem hiding this comment.
I think the <media> part is unclear. I suggest showing examples for images ({image:link}, {image!ref}, etc..) and later say the same work for video and audio (possibly with more examples).
src/document/comment.ml
Outdated
| match Url.from_identifier ~stop_before:false id with | ||
| | Ok url -> Target.Internal (Resolved url) | ||
| | Error exn -> | ||
| (* FIXME: better error message *) |
There was a problem hiding this comment.
It's unclear why this is needed. Couldn't we change the type of `Media's first argument to never hit this case ?
There was a problem hiding this comment.
Trying to turn odoc IDs that contain core types/exception returns an error. Since there is no type including "all ids without core types", we end up with a lot of those "FIXME: better error message", assert false and other failwith "bad"...
In 34ef32c I removed this case since we know Asset identifier won't have core types/exceptions. But this is not fixing it in any other context, which is out of scope from this PR.
src/html/generator.ml
Outdated
| @@ -206,6 +207,26 @@ let text_align = function | |||
|
|
|||
| let cell_kind = function `Header -> Html.th | `Data -> Html.td | |||
|
|
|||
| (* Turns an inline into a string, for use as alternative text in | |||
| images *) | |||
| let rec alt_of_inline (i : Inline.t) = | |||
There was a problem hiding this comment.
I find it weird to allow inline markup in the syntax but then strip it out. The AST could contain just a string.
This markup is used in other backends but I don't think that's going to be used by anyone. If someone wanted to write something richer than an alt text here, why hide it on the most common backend?
There was a problem hiding this comment.
I'm happy "forbidding" marked-up content in alt texts (just like block content is forbidden in inline content), but I'd still want to parse the content.
Indeed, it would be very strange to me if {{image:link}Some {b emphasized} content} were an image with alt text "Some {b emphasized" followed by the text "content}".
We could raise a warning (Warning: '{b ...}' (bold content) is not allowed in '{{:...} ...}' (external link)., and have the AST contain just a string, as you suggest. But do you agree that we should render it as it is rendered now ("Some emphasized content", stripping the {b) ?
src/html/generator.ml
Outdated
| @@ -243,6 +264,55 @@ let rec block ~config ~resolve (l : Block.t) : flow Html.elt list = | |||
| let extra_class = [ "language-" ^ lang_tag ] in | |||
| mk_block ~extra_class Html.pre (source (inline ~config ~resolve) c) | |||
| | Math s -> mk_block Html.div [ block_math s ] | |||
| | Audio (target, content) -> | |||
| let content = inline ~config ~resolve content in | |||
There was a problem hiding this comment.
There's a lot of duplicated code and it's not entirely obvious code. I'd really prefer to have this factorized.
src/xref2/ref_tools.ml
Outdated
| @@ -831,6 +831,11 @@ let resolved3 (r, _, _) = resolved1 r | |||
|
|
|||
| and resolved2 (r, _) = resolved1 r | |||
|
|
|||
| let resolve_asset_reference env (r : Reference.Asset.t) : Asset.t ref_result = | |||
| match r with | |||
| | `Resolved _r -> failwith "What's going on!?" | |||
There was a problem hiding this comment.
Haha I think we can just return r. In the past some things could be linked twice in some complex cases with includes and stuff like that. This could happen again, let's not crash.
There was a problem hiding this comment.
Fixed for reresolving assets in 01109c6
The strange failwith is still there for module and signature references, since it's harder to solve and out of scope from this PR.
| (* The string is scanned right-to-left, because we are interested in right-most | ||
| hyphens. The tokens are also returned in right-to-left order, because the | ||
| traversals that consume them prefer to look at the deepest identifier | ||
| first. *) | ||
| let tokenize location s : token list = | ||
| let tokenize location s : token list * path_prefix option = |
|
I believe all review comments have been addressed! Here are the important changes:
|
panglesd
force-pushed
the
media-in-odoc3
branch
from
b319254
to
c27df3f
Compare
|
Rebased! (and fixed changelog entry for #1185) |
|
I looked at occurrences of images in odoc documentation using embedded html: see this. Not all, but many uses include a way to specify the size of the image. While I think it should be done in another PR, a syntax for specifying the size will be quite important to define! |
jonludlam
force-pushed
the
media-in-odoc3
branch
3 times, most recently
from
5f7712e
to
62663f5
Compare
Due to difference in cmdliner output.
Co-authored-by: Jules Aguillon <[email protected]>
As per the recommended solution of: https://www.w3.org/WAI/PF/HTML/wiki/Media_Alt_Technologies
jonludlam
force-pushed
the
media-in-odoc3
branch
from
62663f5
to
3d20740
Compare
This is a revival of #1005 but with the new asset references from odoc 3.
The main difference for users is that the media target use path instead of odoc references:
{image!path/to/image.jpg}without needing any escaping.See #1005 and #1113 for prior discussions.