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

css: update selectors for odig package title #1290

Merged
merged 2 commits into from
Jan 26, 2025

Conversation

katrinafyi
Copy link
Contributor

@katrinafyi katrinafyi commented Jan 23, 2025

this is for the package index pages with the titles that look like: Package angstrom 0.16.1 issues more….

it appears that the new heading structure is something like this:

<header class="odoc-preamble">
  <h1 id="package-angstrom">
    <a href="#package-angstrom" class="anchor"></a>
    Package angstrom
    <span class="version">0.16.1</span>
    <nav>
      <a href="https://github.com/inhabitedtype/angstrom/issues">issues</a>
      <a href="#package_info">more…</a>
    </nav>
  </h1>

notably, the h1 element doesn't have a .package classname anymore which breaks the styling, reverting the version and links to the large title text size.

the new selectors are borrowed from the odig css.
https://github.com/b0-system/odig/blob/b0e7b144d921d345d49cf4958caab06059739754/themes/odoc.css#L293-L297

this is for the package index pages. 
it appears that the new heading structure is something like this:
```
<header class="odoc-preamble">
  <h1 id="package-angstrom">
    <a href="#package-angstrom" class="anchor"></a>
    Package angstrom
    <span class="version">0.16.1</span>
    <nav>
      <a href="https://github.com/inhabitedtype/angstrom/issues">issues</a>
      <a href="#package_info">more…</a>
    </nav>
  </h1>
```
notably, the h1 element doesn't have a .package classname anymore. the new selectors are borrowed from the odig css.
@jonludlam
Copy link
Member

Perhaps @dbuenzli would like to weigh in here?

@dbuenzli
Copy link
Contributor

I think a bit more context may be needed. With:

> odoc --version
2.4.4
> odig --version
v0.0.9

and odig odoc-theme set odoc-default I see this:
Screenshot 2025-01-23 at 20 14 29

and (for example) odig odoc-theme set odig.default I see this:

Screenshot 2025-01-23 at 20 15 58

I guess @katrinafyi wants to get rid of these oversized links.

A blame on the mentioned lines seems to indicate that the change in odig's odoc.css happened between 4-6 years ago.

I'd say it's not to me to tell what odoc's theme should look like. Perhaps @katrinafyi can make a before/after screenshot to make it easier to odoc's dev to vouch the changes ?

(I'm sorry but css selectors of odig is the last thing I have in my head, well css selectors in general so it's diffcult for me to say, yes this is it :-), also just in case here's the .mld/raw html code that generates these bits.)

@katrinafyi
Copy link
Contributor Author

Hello, thank you both for your comments! Sorry for being a bit short on detail.

Here are the before and after screenshots, both built with odig v0.0.9.

before:

Screenshot_20250124_095041

after:

Screenshot_20250124_100314

I found that the .package nav and .package .version selectors didn't exist within the current HTML, so I assumed they were meant to match an old version of these elements. I phrased the PR in this way, but from the git blame maybe this is not the case - sorry for any confusion.

In any case, I would like the version and links to be made more distinct from the package title. If the current style is the intended odoc style, I can make the changes in a custom theme :)

@jonludlam
Copy link
Member

Thanks both. It seems slightly weird to me that we've got CSS rules that don't apply to anything constructed by this repo, but relatively harmless. I'm happy to merge this, though could I ask @katrinafyi to write a quick changelog entry for it?

@katrinafyi
Copy link
Contributor Author

I've added an entry under the "added" section. Hopefully the wording is okay :)

@jonludlam jonludlam merged commit 2e03395 into ocaml:master Jan 26, 2025
2 checks passed
@jonludlam
Copy link
Member

Thanks!

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.

3 participants