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

Don't hide fields of enum struct variants #87332

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Don't hide fields of enum struct variants #87332

merged 1 commit into from
Jul 23, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Jul 21, 2021 •
edited
Loading

  • The toggle adds visual clutter
  • It's easy to miss that there are fields
  • Tuple variant fields are always shown, so it is inconsistent to hide
    struct variant fields
  • It's annoying to have to click the toggle every time

@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 21, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2021
@camelid camelid added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Jul 21, 2021
@camelid
Copy link
Member Author

camelid commented Jul 21, 2021

Example

pub enum Enum {
    Variant1 { x: i32, y: String },
    Variant2(Vec<u32>, std::path::PathBuf),
}

Output (before)

image

Output (after)

image

@camelid camelid force-pushed the dont-hide-fields branch from 0134918 to 9f724ba Compare July 21, 2021 01:21
@camelid camelid changed the title Don't hide fields of enum variants Don't hide fields of enum struct variants Jul 21, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid camelid force-pushed the dont-hide-fields branch from 9f724ba to 09b9475 Compare July 21, 2021 01:50
GuillaumeGomez
GuillaumeGomez reviewed Jul 21, 2021
View reviewed changes
src/test/rustdoc/toggle-item-contents.rs
@@ -62,8 +62,7 @@ pub struct PrivStruct {
}

// @has 'toggle_item_contents/enum.Enum.html'
// @count - '//details[@class="rustdoc-toggle type-contents-toggle"]' 1
// @has - '//details[@class="rustdoc-toggle type-contents-toggle"]' 'Show fields'
// @!has - '//details[@class="rustdoc-toggle type-contents-toggle"]'
Copy link
Member

Choose a reason for hiding this comment

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

Can you also check the "DOM path" please? !has doesn't cover changes, only that something doesn't exist after all.

Copy link
Member Author

@camelid camelid Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this current test more general than checking a specific DOM path? I feel a bit confused about what you'd like me to change the test to. Could you give an example of what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that the <details> tag was wrapping another tag, but this is not the case. I really don't like that but I guess we can't do better.

@bors

This comment has been minimized.

Sign in to view
@camelid camelid force-pushed the dont-hide-fields branch from 09b9475 to cb9c328 Compare July 22, 2021 19:32
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Don't hide fields of enum struct variants
97623bf 
* The toggle adds visual clutter
* It's easy to miss that there are fields
* Tuple variant fields are always shown, so it is inconsistent to hide
  struct variant fields
* It's annoying to have to click the toggle every time
@camelid camelid force-pushed the dont-hide-fields branch from cb9c328 to 97623bf Compare July 22, 2021 19:52
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 22, 2021

📌 Commit 97623bf has been approved by GuillaumeGomez

@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 Jul 22, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 23, 2021
@JohnTitor
Rollup merge of rust-lang#87332 - camelid:dont-hide-fields, r=Guillau…
fef2afe 
…meGomez

Don't hide fields of enum struct variants

* The toggle adds visual clutter
* It's easy to miss that there are fields
* Tuple variant fields are always shown, so it is inconsistent to hide
  struct variant fields
* It's annoying to have to click the toggle every time
This was referenced Jul 23, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2021
@bors
Auto merge of rust-lang#87400 - JohnTitor:rollup-zbwyuxi, r=JohnTitor
0443424 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#87034 (DOC: fix hypothetical Rust code in `step_by()` docstring)
 - rust-lang#87298 (memorialize Anna Harren in the bastion of the turbofish)
 - rust-lang#87332 (Don't hide fields of enum struct variants)
 - rust-lang#87362 (Make `x.py d` an alias for `x.py doc`)
 - rust-lang#87372 (Move calls to test_main into one function)
 - rust-lang#87373 (Extend HIR WF checking to fields)
 - rust-lang#87376 (Change rustdoc logo to use the full container size)
 - rust-lang#87383 (Add regression tests for the impl_trait_in_bindings ICEs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 377bb73 into rust-lang:master Jul 23, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 23, 2021
@camelid camelid deleted the dont-hide-fields branch July 23, 2021 18:12
@camelid camelid mentioned this pull request Oct 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.
Projects
None yet
Milestone
1.55.0
Development

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