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.

Sign up for GitHub

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

Jump to bottom

[rustdoc] Replace module list items ul/li with dl/dd/dt elements #135641

Merged
merged 4 commits into from
Jan 19, 2025
Merged

[rustdoc] Replace module list items ul/li with dl/dd/dt elements #135641

merged 4 commits into from
Jan 19, 2025

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 17, 2025 •
edited
Loading

@Hywan suggested that rustdoc should use dl,dt and dd HTML tags for listing items on module pages as it matches better what this is (an item and optionally its description). This is a very good idea so here is the implementation.

Also nice side-effect of this change: it reduces a bit the generated HTML since we go from:

This PR shouldn't impact page appearance.

<ul class="item-table">
  <li>
    <div class="item-name">NAME</div>
    <div class="desc docblock-short">THE DOC</div>
  </li>
</ul>

to:

<dl class="item-table">
  <dt>NAME</dt>
  <dd>THE DOC</dd>
</dl>

You can test it here.

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2025

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@notriddle
Copy link
Contributor 

<dl class="item-table">
  <dt>NAME</dt>
  <dd class="docblock-short">THE DOC</dd>
</dl>

Do we still need item-name and desc?

@aDotInTheVoid aDotInTheVoid added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Jan 17, 2025
@GuillaumeGomez
Copy link
Member Author

I suppose we don't indeed.

@GuillaumeGomez
Copy link
Member Author

Updated online docs and removed desc, item-name and docblock-short CSS classes.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Forgot to rebase to get changes from #135499. So now done, all updated as well.

notriddle
notriddle reviewed Jan 18, 2025
View reviewed changes
Copy link
Contributor

@notriddle notriddle left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't want to style <dd> and <dt> tags within the docblocks.

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -242,7 +242,7 @@ h1, h2, h3, h4, h5, h6,
.mobile-topbar,
.search-input,
.search-results .result-name,
.item-name > a,
dt > a,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dt > a,
.item-table dt > a,

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -385,11 +385,11 @@ details:not(.toggle) summary {
code, pre, .code-header, .type-signature {
font-family: "Source Code Pro", monospace;
}
.docblock code, .docblock-short code {
.docblock code, dd code {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.docblock code, dd code {
.docblock code, .item-table dd code {

src/librustdoc/html/static/css/rustdoc.css Outdated
border-radius: 3px;
padding: 0 0.125em;
}
.docblock pre code, .docblock-short pre code {
.docblock pre code, dd pre code {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.docblock pre code, dd pre code {
.docblock pre code, .item-table dd pre code {

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -887,13 +887,13 @@ both the code example and the line numbers, so we need to remove the radius in t
text-align: center;
}

.docblock-short {
dd {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dd {
.item-table dd {

src/librustdoc/html/static/css/rustdoc.css Outdated
overflow-wrap: break-word;
overflow-wrap: anywhere;
}
/* Wrap non-pre code blocks (`text`) but not (```text```). */
.docblock :not(pre) > code,
.docblock-short code {
dd code {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dd code {
.item-table dd code {

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -938,7 +938,7 @@ rustdoc-toolbar {
min-height: 60px;
}

.docblock code, .docblock-short code,
.docblock code, dd code,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.docblock code, dd code,
.docblock code, .item-table dd code,

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -964,7 +964,7 @@ pre, .rustdoc.src .example-wrap, .example-wrap .src-line-numbers {
background: var(--table-alt-row-background-color);
}

.docblock .stab, .docblock-short .stab, .docblock p code {
.docblock .stab, dd .stab, .docblock p code {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.docblock .stab, dd .stab, .docblock p code {
.docblock .stab, .item-table dd .stab, .docblock p code {

src/librustdoc/html/static/css/rustdoc.css Outdated
@@ -1069,7 +1069,7 @@ because of the `[-]` element which would overlap with it. */
.example-wrap .rust a:hover,
.all-items a:hover,
.docblock a:not(.scrape-help):not(.tooltip):hover:not(.doc-anchor),
.docblock-short a:not(.scrape-help):not(.tooltip):hover,
dd a:not(.scrape-help):not(.tooltip):hover,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dd a:not(.scrape-help):not(.tooltip):hover,
.item-table dd a:not(.scrape-help):not(.tooltip):hover,

@GuillaumeGomez
Copy link
Member Author

Good catch! Updated.

@notriddle
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jan 19, 2025

📌 Commit b3865d1 has been approved by notriddle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 19, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2025
@bors
Auto merge of rust-lang#135715 - matthiaskrgr:rollup-9a18sxj, r=matth…
9857284 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#135641 ([rustdoc] Replace module list items `ul`/`li` with `dl`/`dd`/`dt` elements)
 - rust-lang#135703 (Disallow `A { .. }` if `A` has no fields)
 - rust-lang#135705 (Consolidate ad-hoc MIR lints into real pass-manager-based MIR lints)
 - rust-lang#135708 (Some random compiler nits)

Failed merges:

 - rust-lang#135685 (Remove unused `item-row` CSS class)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 85b69bf into rust-lang:master Jan 19, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 19, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2025
@rust-timer
Unrolled build for rust-lang#135641
246fe18 
Rollup merge of rust-lang#135641 - GuillaumeGomez:items-list, r=notriddle

[rustdoc] Replace module list items `ul`/`li` with `dl`/`dd`/`dt` elements

`@hywan` suggested that rustdoc should use `dl`,`dt` and `dd` HTML tags for listing items on module pages as it matches better what this is (an item and optionally its description). This is a very good idea so here is the implementation.

Also nice side-effect of this change: it reduces a bit the generated HTML since we go from:

This PR shouldn't impact page appearance.

```html
<ul class="item-table">
  <li>
    <div class="item-name">NAME</div>
    <div class="desc docblock-short">THE DOC</div>
  </li>
</ul>
```

to:

```html
<dl class="item-table">
  <dt>NAME</dt>
  <dd>THE DOC</dd>
</dl>
```

You can test it [here](https://rustdoc.crud.net/imperio/items-list/std/index.html).

r? `@notriddle`
@GuillaumeGomez GuillaumeGomez deleted the items-list branch January 19, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle left review comments

Assignees

@notriddle notriddle

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @rustbot @rust-log-analyzer @notriddle @bors @aDotInTheVoid