JS sidebar #313
JS sidebar #313
Conversation
The designThe current implementationIn the current simple sidebar implementation a sidebar is specified as an array of objects. Each object is a "sidebar section". A sidebar section can be:
A link list spec can be specified as one of:
Changes to support a JS sidebarIt seems like there are basically two changes needed to support a sidebar like the one we discussed in #302. First: although the current sidebar supports nesting, nested items have to be explicitly specified. There's no way to say: "for each page you find under JavaScript/Global_objects, list out its subpages". So I thought we might be able to let people add an extra property Second: we don't just want to list all the items. We want to have separate lists for constructors, static methods, instance methods, and so on. The first thing I thought of here was tagging: we could let writers filter directories by tag. But then I realized that we already have this information about static methods, constructors, and so on in the JS class front matter. This is already used to build the JS class pages themselves. It seems good to use one data source for this, and to have objects describe their structure explicitly, rather than relying on pages tagging themselves. So I thought, maybe we could have a thing where if you are specifying a list inside foreach, then you already have the context of the containing page. So perhaps in that case you could specify lists indirectly, by referencing a front matter item. So with those together, you could specify a JS sidebar using YAML like this: - title: Tutorials
children:
- chapter_list: /content/en-US/javascript/tutorials/javascript-beginners
- chapter_list: /content/en-US/javascript/tutorials/javascript-guide
- chapter_list: /content/en-US/javascript/tutorials/javascript-intermediate
- chapter_list: /content/en-US/javascript/tutorials/javascript-advanced
- title: Reference
children:
- title: Global objects
directory: /content/en-US/javascript/reference/global_objects
foreach:
- title: Constructor
pages:
- front_matter_item: class_constructor
- title: Static methods
directory:
front_matter_item: static_methods
- title: Instance methods
directory:
front_matter_item: instance_methods
- title: Static properties
directory:
front_matter_item: static_properties
- title: Instance properties
directory:
front_matter_item: instance_properties
- title: Expressions and operators
directory: /content/en-US/javascript/reference/expressions_and_operators
- title: Statements and declarations
directory: /content/en-US/javascript/reference/statements_and_declarationsThe interesting bit is obviously "Global objects", which says:
Reasons I like this approach:
Does it make sense though? If not, what does? :) |
|
This PR has three commits. The third one is comment-only, but: The first commit does some refactoring of the link list/related content stuff, to make it easier to make this change. A lot of this is just trying to make consistent names for things, some of it is making the code more consistent (for example, having The only substantive change in this commit, I think, is to remove the toggle to include or exclude short descriptions: this was only included to try to make sidebar-building faster. But there are going to be much better ways to do that, if it's needed, and it made things more complicated. I've verified that the JSON built with this commit is the same as the previous JSON, except that short descriptions are now always included. The second commit adds the bits to support a more complex JS sidebar, and also adds an example using the JS docs currently in there. One thing is: some of these front matter items are optional. In this commit I'm tolerant of missing front matter items, just ignoring them. I think this is OK, because recipe linting should already catch missing mandatory front matter items, and it should be possible to reference optional ones. The example sidebar is specified like: - title: Reference
children:
- title: Global objects
directory: /content/en-US/javascript/reference/classes
foreach:
- title: Constructor
pages:
- front_matter_item: class_constructor
- title: Static methods
directory:
front_matter_item: static_methods
- title: Instance methods
directory:
front_matter_item: instance_methods
- title: Static properties
directory:
front_matter_item: static_properties
- title: Instance properties
directory:
front_matter_item: instance_properties...and that generates JSON like: "related_content": [
{
"title": "Reference",
"content": [
{
"title": "Global objects",
"content": [
{
"title": "BigInt",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt",
"short_description": "<p><strong><code>BigInt</code></strong> is a built-in object that provides a way to represent whole numbers larger than 253 - 1, which is the largest number JavaScript can reliably represent with the <a href=\"/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number\"><code>Number</code></a> primitive. <strong><code>BigInt</code></strong> can be used for arbitrarily large integers.</p>",
"content": [
{
"title": "Constructor",
"content": [
{
"title": "BigInt() constructor",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/BigInt",
"short_description": "<p>The <strong><code>BigInt()</code></strong> constructor is used to create <a href=\"/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt\"><code>BigInt</code></a> objects.</p>"
}
]
},
{
"title": "Static methods",
"content": [
{
"title": "BigInt.asIntN()",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/asIntN",
"short_description": "<p>The <strong><code>BigInt.asIntN</code></strong> static method is used to wrap a <code>BigInt</code> value to a signed integer between -<code>2width-1</code> and <code>2width-1-1</code>.</p><p>The source for this interactive example is stored in a GitHub repository. If you'd like to contribute to the interactive examples project, please clone <a href=\"https://github.com/mdn/interactive-examples\">https://github.com/mdn/interactive-examples</a> and send us a pull request.</p>"
},
{
"title": "BigInt.asUintN()",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/asUintN",
"short_description": "<p>The <strong><code>BigInt.asUintN</code></strong> static method is used to wrap a <code>BigInt</code> value to an unsigned integer between between 0 and <code>2width-1</code>.</p>"
}
]
},
{
"title": "Instance methods",
"content": [
{
"title": "BigInt.prototype.toLocaleString()",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/toLocaleString",
"short_description": "<p>The <strong><code>toLocaleString()</code></strong> method returns a string with a language-sensitive representation of this <code>BigInt</code>.</p>"
},
{
"title": "BigInt.prototype.toString()",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/toString",
"short_description": "<p>The <strong><code>toString()</code></strong> method returns a string representing the specified <a href=\"/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt\"><code>BigInt</code></a> object. The trailing \"n\" is not part of the string.</p><p>The source for this interactive example is stored in a GitHub repository. If you'd like to contribute to the interactive examples project, please clone <a href=\"https://github.com/mdn/interactive-examples\">https://github.com/mdn/interactive-examples</a> and send us a pull request.</p>"
},
{
"title": "BigInt.prototype.valueOf()",
"short_title": null,
"mdn_url": "/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt/valueOf",
"short_description": "<p>The <strong><code>valueOf()</code></strong> method returns the wrapped primitive value of a <a href=\"/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt\"><code>BigInt</code></a> object.</p><p>The source for this interactive example is stored in a GitHub repository. If you'd like to contribute to the interactive examples project, please clone <a href=\"https://github.com/mdn/interactive-examples\">https://github.com/mdn/interactive-examples</a> and send us a pull request.</p>"
}
]
}
]
}
]
}
]
}
](the short description Markdown is obviously broken in these files :() We will need changes to stumptown-renderer to render this JSON properly, but this seems like it ought to be a reasonable basis for the kind of sidebar we want. |
|
@wbamberg probably a stupid question, but what is the best way to test this? |
There was a problem hiding this comment.
Thanks for putting this together, Will. In substance, I'm happy with this. There were a few things that I had a hard time following on first read, so I've made a few comments in the interest of making this easier to understand the next time I work with those code, but I'm happy with what this does.
| foreach: | ||
| - title: Constructor | ||
| pages: | ||
| - front_matter_item: class_constructor |
There was a problem hiding this comment.
I'm worried this is a little bit of a bike shedding thing, but can I suggest renaming front_matter_item? On first read, I thought front_matter_item was a little ambiguous; it might be reasonable to read it as doing something to the front matter in each item, rather than reading from it. Some alternative names I came up with:
from_front_matter_keyfrom_ingredient(though this suggests changing the values to correspond to recipes, such asdata.class_constructor)from_front_matter_item
There was a problem hiding this comment.
I've gone with the first of these.
| * Get a single link (title and URL) from a file path. | ||
| * If `includeShortDescriptions` is `true`, and the file includes short description, | ||
| * then include that, too. | ||
| * Given a path to an MD file containing the content for a page, |
There was a problem hiding this comment.
This is definitely not germane, so you can stop reading here. "An MD file" broke my brain. Do I read this as "an em-dee file"? Or "an markdown file"? I got hung up on both. If it were me, I'd go with "a .md file" or "a markdown file" to escape my self-inflicted nonsense. 😆
There was a problem hiding this comment.
Updated to "a Markdown file" :).
| .filter(entry => !entry.isDirectory()) | ||
| .filter(entry => entry.name.endsWith(".md")) | ||
| .map(entry => path.join(itemDirectory, entry.name)); | ||
| let items = filenames |
There was a problem hiding this comment.
| let items = filenames | |
| const items = filenames |
| ); | ||
| content = content.filter(e => !!e); | ||
| if (content.length !== 1) { | ||
| function buildLinkItem(foreach, itemDirectory) { |
There was a problem hiding this comment.
I'm confused by the signature here and the way buildLinkItem actually gets called. The comment implies that foreach is optional, but it's only ever called with false or an object. Reading the code, I get the effect of false, but it's still kinda weird that it's sort of a flag for itemDirectory or an array.
Maybe some more description in the comment or an example or two would help. Or maybe we could try a signature like buildLinkItem(itemDirectory, foreach = []) instead.
There was a problem hiding this comment.
I hope this is clearer now, I have changed the way this function is called. I agree it was weird.
Sorry @chrisdavidmills , not a stupid question. This PR implements the stuff in stumptown-content that transforms the YAML written by authors into the JSON that the renderer can use to build a sidebar. So to test it you can just run For example, you ought to see a file at /packaged/en-US/javascript/reference/classes/bigint/bigint.json, and that file ought to contain a You could change the /content/en-US/related_content/javascript.yaml and expect to see corresponding changes in the JSON. However this is a bit tricky because we have so few docs in the tree at the moment, and build-json will complain about things being missing if you reference nonexistent files or directories in your YAML. To see the actual rendered sidebar we'll need corresponding changes in stumptown-renderer. I will get on with that. From a review point of view I'm most interested in questions like:
|
|
To see the rendered sidebar, I pushed a really minimal change to stumptown-renderer, here: https://github.com/mdn/stumptown-renderer/tree/js-sidebar. Assuming you have the branch containing the stumptown-content changes (i.e. this PR) checked out at, say, ~/git/wbamberg/stumptown-content, then you ought to be able to use it by doing something like: ...then opening the bigint page.
It's rough at the moment: in particular I spent far too long trying to find a nice way to only open From this it looks to me like we ought to define |
|
Thanks for the details @wbamberg — this makes things much easier. I'm having a problem unfortunately. I've got stumptown-content cloned locally, with this PR pulled into a branch. Then I've got stumptown-renderer cloned locally. I've made sure they are both up to date, and done npm install on both of them. On the content side, I've run When I follow your yarn instructions over on the renderer side, it fails on (well, my equivalent is I've put the output into a pastebin: https://pastebin.com/ZbYXrWCK Any thoughts? |
|
@peterbe , do you have any insight into Chris's issue: #313 (comment) ? Unfortunately the stumptown-renderer build process is a black box to me :(. |
There's more to say about this issue, but one note: I found a horrible bug in this code: although it wasn't related actually to the way buildLinkItem is called, it made me think it would be better to do something simpler here, so I have changed that. |
|
I've just pushed 0aaa9d1, which adds a ton of extra JS content. I wasn't sure if this was a good idea, because the new content isn't properly structured (although it's good enough to pass the linter and build properly). But I think it's quite helpful to see what the sidebar looks like when there's a bit more stuff in there, so decided on balance it was better to include it. |
|
I wanted to review this, but I get the same error that Chris got. I've tried updating my npm, yarn, node and stuff and also attempted to re-install all the stumptown stuff from scratch but it seems to fail and I'm not sure how much further time I want to spend on debugging this. :/ |
You need to use
So the error is something that ought to be very rock solid and it's not our code. You should be able to do |
|
I tried this again today, after reinstalling node/npm to make sure I'd get the latest version, and it works! I was able to build everything, and get the stumptown-content rendered at localhost:3000. I like the look of the rendered sidebar - nice and simple and cruft-free, and the way it works seems to make sense. I also like the look of the top-level JS sidebar JSON - again, it makes sense to me, I can read it easily, and I think soemthing similar would work for HTML, CSS, etc. I suppose the main question I have is around sidebar customization, and what happens when you want to mix something like the very specific BigInt sidebar, with the general top level JS sidebar. For example, what happens if you want to include something custom in the sidebar that isn't just inferred from the meta data, etc? And what would happen if we wanted to for example show the specific bigint stuff up the top of the sidebar on the BigInt pages, but then lower down show the more generic stuff if you want to navigate to other reference items, or tutorials, etc.? Would there be a combination of auto-generated sidebar stuff, plus a section that is defined in the JSON? Or am I just getting confused as to the purpose of this stuff? |
In this PR, there is one sidebar for all the JS docs. The only difference between the sidebar for Can you me more specific about what you might want to have in "the very specific BigInt sidebar", that isn't in here, and is in the current
As it says in #313 (comment), this implements the first of the options from #313, which does not float the section for a particular object. I could have a go at doing this if we want to use this design instead, but of course it will take some more time. |
Nope, no need - this answers my questions Will, thanks! With the time between looking at this all, I lost context. Sorry about that, and thanks for bringing me up to speed again. |
I like the three options of directory, chapter list, explicit list of pages! Great work!
I was looking at other sidebars to see if there are other types and I think the only option that isn't possible is a plain link? Maybe I'm missing something, though. I believe certain sidebars have simple links at the top level, but I can't add something like the first two lines here, right? - title: JavaScript
- link: /content/en-us/javascript
- title: Tutorials
children:
- title: Complete beginners
pages:
- /content/en-US/learn/getting_started_with_the_web/JavaScript_basics
- /content/en-US/learn/javascript/Building_blocks
- /content/en-US/learn/javascript/First_steps
- /content/en-US/learn/javascript/Objects
- chapter_list: /content/en-US/javascript/guide/javascript_guide.yamlMaybe it isn't needed at this stage. Just a thought.
I think it looks alright. More fiddling with this would of course help to make it more solid, but I played with it a bit and added some things and it did work.
Seems so. I think we've agreed to stop doing some of the advanced styling and adding badges, so it looks good to me :) |
Fix case in chapter list Co-Authored-By: Florian Scholz <[email protected]>
This PR contains an implementation of some extensions to the basic sidebar syntax, to support a JS sidebar like the one discussed in #302 - specifically, the first option.
I'm going to have one comment on the design it tries to implement, and another on the code changes in this PR.