Conversation
|
is this just a reopening of #60778 ? |
|
No, I made some changes. For instance, there is no more global checkbox, just a title for sub-settings. |
|
So to be clear, this PR changes the current "Auto-hide item declarations." into a set of preferences for each item type? Can you update the top-level comment with an answer to this question?
Personally, I never really found it all that useful to have a global hide setting for item declarations here so I would not want this feature -- I do generally work on at least a 15" screen though so in that sense I have relatively large screen real estate. Is this feature wanted by folks? And in the "yes I'd use this" not the "well, why not" sense? |
No back compatibility if this is your question.
Debated on the previously opened PR.
I've been asked to do this a lot of times actually (IRL and IRC). I personally don't care much about this feature but people seems to want to have a better control over the types declaration view. |
But, uh, what happens? Presumably we don't just error out and not run at all...
Do you have a link? I don't see any such discussion on #60778...
I would personally echo @QuietMisdreavus's concerns on #60778 that this does not seem to really carry it's weight. However, if there is indeed lots of demand, it also seems like a 'not bad' idea. |
It doesn't take it into account. That's pretty much it.
Seems like it wasn't there... Multi-channels communications are bad...
Like I said, I didn't do it just because. People asked me for it and at some point I just said ok and did it. |
JohnCSimon
added
the
S-waiting-on-author
|
I'm going to r? @QuietMisdreavus here as a rustdoc team member, I don't personally feel comfortable driving this |
JohnCSimon
added
S-waiting-on-review
|
Ping from triage - |
|
Ping from triage. @QuietMisdreavus any updates on this? Thanks. |
|
☔ The latest upstream changes (presumably #65716) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Pinging from triage: |
GuillaumeGomez
force-pushed
the
rustdoc-sub-settings
branch
from
17e555d to
40f3e48
Compare
|
Rebased. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
GuillaumeGomez
force-pushed
the
rustdoc-sub-settings
branch
from
40f3e48 to
79956b9
Compare
|
Fixed the issue. |
|
Ping from triage |
|
r? @kinnison |
kinnison
left a comment
kinnison
left a comment
There was a problem hiding this comment.
In addition to the pluralisation comment I have some concerns that we are not testing the generation of this content at all.
It would probably behoove us to ensure there's at least a basic test checking for the presence of the settings in the generated output.
src/librustdoc/html/render.rs
Outdated
| format!( | ||
| "<div class='setting-line'>\ | ||
| <div class='title'>{}</div>\ | ||
| <div class='sub-setting'>{}</div> |
There was a problem hiding this comment.
Given that this div contains potentially more than one sub-setting, I wonder if it ought to be sub-settings ?
| <div class='sub-setting'>{}</div> | |
| <div class='sub-settings'>{}</div> |
| transform: translateX(19px); | ||
| } | ||
|
|
||
| .setting-line > .sub-setting { |
There was a problem hiding this comment.
To match the change to sub-settings you'd need:
| .setting-line > .sub-setting { | |
| .setting-line > .sub-settings { |
|
We have a UI server but the integration with highfive is buggy and needs to be fixed. (Maybe when @pietroalbini has some time, we could take a look to understand what's going on) |
|
Updated. |
|
Regarding some basic regression testing, could we do a basic rustdoc-ui test using |
|
It allows to check the DOM, so it depends what you want to check? |
|
I was just thinking that a basic check demonstrating that the settings have made it out in a form we were expecting -- that way if the shape is changed in the future, there's a regression test reminding people to re-check it and update the test if it's good. I wouldn't block a merge on that but you were saying you wanted more tests :D |
|
It's more UI tests then. It's scheduled but I need to have highfive fully working on this so we don't have bad surprises. |
|
Okay, if you want to make a note (issue?) for the need for the test, and merge the fix in the meantime, I'm good with that. |
|
📌 Commit 8784b07 has been approved by |
bors
added
S-waiting-on-bors
…, r=kinnison [rustdoc] add sub settings This PR is to give a finer control over what types are automatically expanded or not as well as the possibility to add sub-settings in the settings page.  r? @Mark-Simulacrum
…, r=kinnison [rustdoc] add sub settings This PR is to give a finer control over what types are automatically expanded or not as well as the possibility to add sub-settings in the settings page.  r? @Mark-Simulacrum
…, r=kinnison [rustdoc] add sub settings This PR is to give a finer control over what types are automatically expanded or not as well as the possibility to add sub-settings in the settings page.  r? @Mark-Simulacrum
Rollup of 5 pull requests Successful merges: - #63793 (Have tidy ensure that we document all `unsafe` blocks in libcore) - #64696 ([rustdoc] add sub settings) - #65916 (syntax: move stuff around) - #66087 (Update some build-pass ui tests to use check-pass where applicable) - #66182 (invalid_value lint: fix help text) Failed merges: r? @ghost
Rollup of 5 pull requests Successful merges: - #63793 (Have tidy ensure that we document all `unsafe` blocks in libcore) - #64696 ([rustdoc] add sub settings) - #65916 (syntax: move stuff around) - #66087 (Update some build-pass ui tests to use check-pass where applicable) - #66182 (invalid_value lint: fix help text) Failed merges: r? @ghost
9 participants
This PR is to give a finer control over what types are automatically expanded or not as well as the possibility to add sub-settings in the settings page.
r? @Mark-Simulacrum