-
Notifications
You must be signed in to change notification settings - Fork 476
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
Add support for CollapsedDocStrings
in @meta
block
#2394
Conversation
0af4abd
to
f92cf71
Compare
With respect to #2282, this PR does not implement a global flag for collapsing all docstrings. It's not something I'm interested in, although it wouldn't be terribly hard to add it in (but definitely more work to document and test) |
DocStringsCollapsed
in @meta
blockDocStringsCollapsed
in @meta
block
f92cf71
to
0e034e5
Compare
0e034e5
to
cd9d476
Compare
I added collapse-by-default to the Internal API pages, e.g., https://documenter.juliadocs.org/previews/PR2394/lib/internals/anchors/ Interestingly (and unlike in my previous local testing), I'm finding that it doesn't seem completely reliable. Maybe I'll have to revise the JS a bit. It might be running too early or something. |
50b7c83
to
36d7d33
Compare
326fe8e
to
964f67e
Compare
One other thing: I'm a little bit skeptical of putting too many writer-specific options into the top-level scope of an at-meta block. Should we have some way to namespace writer-specific options like this? Not sure what the best syntax would be, but maybe something like: Writer.HTML = begin
CollapsedDocStrings = true
end |
12e0888
to
2768fac
Compare
I have no strong feelings on this either way. Although I'd probably keep it flat, mostly because Assuming I can make get the |
9d34fcb
to
c303d45
Compare
assets/html/js/metadata.js
Outdated
let meta = $("div[data-docstringscollapsed]").data(); | ||
|
||
if (meta.docstringscollapsed) { | ||
$("#documenter-article-toggle-button").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me. It still shows the close-animation after the page is loaded. I don't mind it that much, but @mortenpi might still prefer if we speed it up.
@Hetarth02 Do you know what to add to control the animation speed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I figured it out: e5b84d3
The currently deployed preview documentation disables the animation completely. Better? I'm okay with either.
| `A` | ✓ | 10.00 | | ||
| `BB` | ✓ | 1000000.00 | | ||
| object | implemented | value | | ||
| ------ | ----------- | ---------- | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little unrelated to the PR, but I just kept it in
Okay, this is back on track now, and could be merged in principle. We can still tweak this, of course, As before, https://documenter.juliadocs.org/previews/PR2394/lib/internals/anchors/ (and the other "internals" pages) demo the feature. |
4132356
to
8999adb
Compare
assets/html/js/metadata.js
Outdated
let meta = $("div[data-docstringscollapsed]").data(); | ||
|
||
if (meta.docstringscollapsed) { | ||
$("#documenter-article-toggle-button").trigger({ | ||
type: "click", | ||
noToggleAnimation: true, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing probably needs to be in a $(document).ready(function () { ... }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it! https://documenter.juliadocs.org/previews/PR2394/ should now have that change. Of course, all I can say is that it continues to work for me, so you'll have to let us know :-)
I don't think the "Showcase" page is a good place to have all docstrings collapsed, as it only has a few interspersed docstrings, and the main text explicitly references features from within the docstring. The "Internals" pages seem more sensible, so I'm only leaving those.
6cff39e
to
9f93420
Compare
9f93420
to
4be8288
Compare
With the fixes, it looks like it works now! @Hetarth02, are you still seeing the mobile search/sidebar issue you mentioned on Slack? |
@mortenpi I just checked, everything seems to be working fine. |
DocStringsCollapsed
in @meta
blockCollapsedDocStrings
in @meta
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goerz I took the liberty of pushing a DocStringsCollapsed
-> CollapsedDocString
bikeshed. Happy to revert it if you prefer DocStringsCollapsed
. From my side, I'm happy if we merge this!
I have no preferences on the name of the option. Looks good to me! |
Then let's merge it! Thank you @goerz and @Hetarth02 for iterating on this! |
I'm sorry. How is this keyword supposed to be used? Searching the |
Are you using the If yes, then the relevant documentation would be here: https://documenter.juliadocs.org/dev/man/syntax/#@meta-block and an example is here: https://github.com/JuliaDocs/Documenter.jl/blob/master/docs/src/lib/internals/anchors.md (raw) |
That’s referring to the feature of being able to collapse and expand docstrings manually |
Setting
DocStringsCollapsed = true
in the@meta
block of a particular page essentially clicks the "Collapse all docstrings" in the navigation bar after the page loads, collapsing all docstrings on that page.I think the approach of just having JavaScript click the collapse button is the right one here: First, I'm not sure that it would be possible to write out the page in a collapsed state, or at least not without doing some pretty gnarly rewrites of the
HTMLWriter
. More importantly, rendering the page expanded and then collapsing it with JavaScript seems like the right thing to do from an accessibility standpoint: We'd want the page to still be useful without JavaScript. I've been known to access API documentation in the terminal withw3m
, so not relying on JavaScript to render the core content is something I actually care about. Screenreaders probably also wouldn't like if all docstrings initially rendered ashidden
.Closes #2282