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

Annotate code that is following or followed by other code #863

Closed

Conversation

favonia
Copy link
Contributor

@favonia favonia commented Jun 8, 2022

Closes #696. The goal of this PR is to make

(** [f][ ][x] *)

look like

(** [f x] *)

with the default theme. The motivation is to write {!val:f}[ x] so that f is linked but the text still looks like [f x].

Technical details

This PR annotates adjacent <code> phrases with CSS classes following-code and followed-by-code so that one can turn off the padding between <code> via CSS. The comment [f][ ][x] will generate something like this:

<code class="followed-by-code">f</code><code class="following-code followed-by-code"> </code><code class="following-code">x</code>

and then the padding can be turned off using the CSS selectors code.following-code and code.followed-by-code. See the changes to src/odoc/etc/odoc.css. The annotation algorithm does not change the asymptotic complexity and I did not notice obvious slowdown. However, a serious benchmarking might still be useful.

Caveats

  1. Due to Indentation changes rendering of <pre> ocsigen/tyxml#287, tyxml would insert spaces when indent is true, but it's no longer odoc's fault.
  2. I did not check how other backends can benefit from the new annotations, but at very least it would not be worse. The LaTeX backend, for example, could benefit from them.

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 8, 2022

The motivation is to write {!val:f}[ x] so that f is linked but the text still looks like [f x].

I don't understand. This is already the case.

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022

The motivation is to write {!val:f}[ x] so that f is linked but the text still looks like [f x].

I don't understand. This is already the case.

The current default CSS theme contains this fragment:

p code,
li code {
  background-color: var(--li-code-background);
  color: var(--li-code-color);
  border-radius: 3px;
  padding: 0 0.3ex;
}

which introduces a 0.3ex padding on both sides of the code. Here's a real example from one of the libraries I wrote (except that I darkened the background for clarity):

image

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 8, 2022

The current default CSS theme contains this fragment:

Well by all means fix the css (for example odig's default stylesheet doesn't suffer from these defects) but I don't think there's the need to complicate the HTML generator for that.

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022

I see. I'm not sure how people want to change the default theme, then. I don't think it's technically possible to select <code> that is adjacent to other <code> using only CSS. Therefore, either we should avoid the padding altogether (e.g., the themes used by odig and the OCaml website v3 did that) or the CSS theme would need some help. On the other hand, I don't feel the code is that complicated---it's mostly just passing context along. In case it helps, I could further minimize the PR by making context an optional argument.

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 8, 2022

To be clear your classes seem to be reinventing the + CSS selector.

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022

To be clear your classes seem to be reinventing the + CSS selector.

+ is close but I don't think it would work because, for example, the <code> in {!val:f} is wrapped inside <a> and thus the inner <code> is not a sibling of the <code> generated by [ x]. In general there could be arbitrarily many layers of styles (bold, italic, etc.). That said, maybe some CSS guru can help us out.

@Octachron
Copy link
Member

That sounds like a quite complicated temporary workaround to avoid implementing references inside code block (e.g [{!val:f} x]). Is that temporary workaround worth the implementation and future-compatibility cost?

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 8, 2022

I may not be a CSS "guru" but I have lost enough useless hours of my life into it to tell you that this:

<code class="followed-by-code">f</code><code class="following-code followed-by-code"> </code><code class="following-code">x</code>

is the beginning of a nightmare.

If possible wouldn't adding a class on the a indicating that this is an id reference and/or swap the <a> and <code> be a simpler option ?

Also, @Octachron's comment.

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022

Fair enough. I think we can use + to do half of it, but I don't know how to control the padding before a series of code phrases. The issue is that there's no adjacent sibling combinator in the opposite direction as far as I know.

And yes, supporting references within code (but still rendering records as they are) would be wonderful!

@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 8, 2022

The issue is that there's no adjacent sibling combinator in the opposite direction as far as I know.

I'm pretty sure you could manage to make the successor background bleed into the precessor so as to cover its right round corners via a judicious use of box-shadow and probably a z-index.

@favonia
Copy link
Contributor Author

favonia commented Jun 8, 2022

Alright. I am closing this while searching for other solutions. Just to clarify, do the developers prefer links within code (which might mess up syntax highlighting...?) or magical swapping of <a> and <code>?

@favonia favonia closed this Jun 8, 2022
@favonia favonia deleted the remove-space-between-adjacent-code branch June 8, 2022 21:34
@dbuenzli
Copy link
Contributor

dbuenzli commented Jun 9, 2022

That sounds like a quite complicated temporary workaround to avoid implementing references inside code block (e.g [{!val:f} x]). Is that temporary workaround worth the implementation and future-compatibility cost?

Just FTR the issue that tracks that is #756

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.

Collapse padding between consecutive code fragments
3 participants