Render JS sidebar #370

wbamberg · 2020-03-03T22:22:19Z

This updates the renderer so it can render the JS sidebar discussed in mdn/stumptown-content#302 and implemented on the content side in mdn/stumptown-content#313.

Changes:

tell stumptown-renderer what to do about class_constructor ingredients. This is not related to the sidebar change itself, but is needed for us to render JS class pages at all.
update the sidebar rendering code. Before this change it expect a sidebar to contain only three levels. But the JS sidebar can and does include more levels than that, so this change should support arbitrary levels. Of course nesting too deep would make the sidebar unusable, but I think the author of the sidebar should have the responsibility of avoiding that.
update the sidebar styling: make it sticky and scrollable. We don't really need this but I think it is much better like this. This sidebar can get quite long, and having to scroll the page to scroll the sidebar seems broken. Note that the Azure docs, which we're partly using as a model for this, does the same: https://docs.microsoft.com/en-us/cli/azure/cdn/custom-domain?view=azure-cli-latest, as does w3schools.
update to the latest stumptown-content

I'd like to scroll the sidebar to the section containing the current page, but I'm not sure where to put that code in the renderer. I'd be very happy to get a pointer for that, but I'd also be OK to do this in a follow up.

wbamberg · 2020-03-04T04:13:03Z

~~Sigh. I always forget to sign commits for this repo. What's the best way for me to fix it at this stage?~~ OK that seemed to work.

…lements

peterbe

Mostly minor things of nitty/stylistics with JSX etc. In fact, to back up my arguments I tested it in a local branch. So I'll send that over as a PR to your PR.

peterbe · 2020-03-05T18:15:14Z

client/src/document.js

-
-function SidebarLeaflets({ node }) {
+function SidebarLeaf({ parent }) {
+  const titleNode = <h3>{parent.title}</h3>;


Why make a variable of this? It's not changing and it's not used repeatedly. I think you should just put the <h3>{parent.title}</h3> directly into line 135. And if that's not possible, if this is a hack due to mutations or something, then it needs to be why-explained with a code comment.

This was in the original code: https://github.com/mdn/stumptown-renderer/blob/master/client/src/document.js#L113.

peterbe · 2020-03-05T18:22:36Z

client/src/document.js

+      return (
+        <li
+          key={childNode.uri}
+          className={childNode.isActive ? "active" : undefined}


Suggested change

className={childNode.isActive ? "active" : undefined}

className={childNode.isActive && "active"}

that way you don't need to mention undefined. This works the same way and I think it looks cleaner

This was in the original code: https://github.com/mdn/stumptown-renderer/blob/master/client/src/document.js#L149.

Sorry if it seemed like I "attacked" your diff. Yes, I should have caught that in the original contribution when this was introduced. Sorry.

peterbe · 2020-03-05T18:24:53Z

client/src/document.js

+  return (
+    <details open={node.open}>
+      <summary>
+        {node.uri ? <a href={node.uri}> {node.title}</a> : node.title}


Suggested change

{node.uri ? <a href={node.uri}> {node.title}</a> : node.title}

{node.uri ? <Link to={node.uri}>{node.title}</Link> : node.title}

peterbe · 2020-03-05T18:30:24Z

client/src/document.js

+      <summary>
+        {node.uri ? <a href={node.uri}> {node.title}</a> : node.title}
+      </summary>
+      <ol>{listItems}</ol>


I think it's a bit of an anti-pattern in JSX to create JSX-node variables instead of inlining them. I'd much rather this was:

<ol> {node.content.map(childNode => { if (childNode.content) { return ( <li key={childNode.title}> <SidebarLeaflets node={childNode} /> </li> ); } else { return ( <li key={childNode.uri} className={childNode.isActive && "active"} > <Link to={childNode.uri}>{childNode.title}</Link> </li> ); } })} </ol>

One of the motivations is that it immediately tells the code-reader that there's no reason why we're looping through it first. Otherwise, I might ask myself "Why did this need to be done first? Is it keeping a counter? What's the hack?"
The other reason it's better to not define jsx-node variables is the location of them. Now you have space between two different concerns and it's can cause the code-reader to have to jump around more than necessary:

const listItems = { CONCERN 1 } return <div> { CONCERN 2 } { CONCERN 3 } { CONCERN 4 } { CONCERN 5 } { CONCERN 1 } </div>

This was in the original code too: https://github.com/mdn/stumptown-renderer/blob/master/client/src/document.js#L158.

peterbe · 2020-03-05T18:44:35Z

client/src/document.js

+        // walk through the tree, annotating its nodes:
+        // * `isActive` if a node contains the link to the current page
+        // * `open` if a node is on the path to the `isActive` node
+        function setOpenNodes(node, pathname) {


Stylistically, I don't like that it's called setOpenNodes and then it's used as a getter function, ie. you care about its return value.

Creating functions on-the-fly isn't great for performance but the effect is unnoticeable at this scale. But I think it's worth keeping fast-by-design in mind if this is ever going to scale.
The only variable here is the location.pathname which gets copied even though it's available as a closure.

The purpose of the function is to mutate the object and continue doing so recursively. Right?

peterbe · 2020-03-05T18:58:57Z

I'd like to scroll the sidebar to the section containing the current page, but I'm not sure where to put that code in the renderer. I'd be very happy to get a pointer for that, but I'd also be OK to do this in a follow up.

Can you explain that? Perhaps my screen is just too large to be able to see how that would work?
But I think the ultimate answer is to do something like this:

useEffect(() => {
  const activeElement = document.querySelector('.sidebar li.active');
  if (activeElement && activeElement.scrollIntoView) { activeElement.scrollIntoView('smooth') }
}, [location]);

kinda

* fixes to js-sidebar pr * simplify * cleanup II

…sr code

wbamberg · 2020-03-06T18:22:32Z

This adds the isActive and open calculation back into the SSR code. We could simplify it a bit - in particular I think it would be totally safe to replace https://github.com/mdn/stumptown-renderer/pull/370/files#diff-80f9f740bfb8aeab2cbf6e6ef4165b0dR44-R54 with just https://github.com/mdn/stumptown-renderer/pull/370/files#diff-80f9f740bfb8aeab2cbf6e6ef4165b0dR52-R53.

We could also perhaps rename open to isOpen, to be consistent.

wbamberg · 2020-03-06T18:23:04Z

But I think the ultimate answer is to do something like this:

Yes, that looks exactly what I'm looking for! Haven't tried it yet though.

peterbe · 2020-03-06T18:25:12Z

But I think the ultimate answer is to do something like this:

Yes, that looks exactly what I'm looking for! Haven't tried it yet though.

Let's tackle it later then. Should be pretty straight forward.

peterbe

Let's do it!

wbamberg · 2020-03-06T18:33:52Z

Thanks Peter!

wbamberg requested a review from peterbe March 3, 2020 22:22

wbamberg force-pushed the js-sidebar branch from 61e5b39 to 0ddba1f Compare March 4, 2020 05:25

Will Bamberg added 5 commits March 3, 2020 21:26

Very basic sidebar rendering

58dc397

Open details elements to show active page; support links in summary e…

33496d0

…lements

Make sidebar sticky, fixed-height, and scrollable

db330ed

Add a comment

b5a912b

Update stumptown-content submodule

0ddba1f

peterbe suggested changes Mar 5, 2020

View reviewed changes

peterbe and others added 3 commits March 6, 2020 09:34

fixes to js-sidebar pr (#375)

4a8e51b

* fixes to js-sidebar pr * simplify * cleanup II

Calculate the active and open attributes for related_content in the s…

f741da1

…sr code

Fix variable name

541a169

peterbe approved these changes Mar 6, 2020

View reviewed changes

peterbe merged commit 162994c into master Mar 6, 2020

wbamberg mentioned this pull request Mar 6, 2020

Support JS sidebar mdn/stumptown-content#302

Closed

peterbe deleted the js-sidebar branch July 2, 2020 15:30

fiji-flo pushed a commit that referenced this pull request Jan 20, 2022

ref #370, live example themes (forces white background)

66a1184

fiji-flo pushed a commit that referenced this pull request Feb 14, 2022

ref #370 white background added to more iframes

eb61df9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render JS sidebar #370

Render JS sidebar #370

wbamberg commented Mar 3, 2020 •

edited

Loading

wbamberg commented Mar 4, 2020 •

edited

Loading

peterbe left a comment

peterbe Mar 5, 2020

wbamberg Mar 5, 2020

peterbe Mar 5, 2020

wbamberg Mar 5, 2020

peterbe Mar 6, 2020

peterbe Mar 5, 2020

peterbe Mar 5, 2020

wbamberg Mar 5, 2020

peterbe Mar 5, 2020

peterbe commented Mar 5, 2020

wbamberg commented Mar 6, 2020

wbamberg commented Mar 6, 2020

peterbe commented Mar 6, 2020

peterbe left a comment

wbamberg commented Mar 6, 2020

	className={childNode.isActive ? "active" : undefined}
	className={childNode.isActive && "active"}

	{node.uri ? <a href={node.uri}> {node.title}</a> : node.title}
	{node.uri ? <Link to={node.uri}>{node.title}</Link> : node.title}

Render JS sidebar #370

Render JS sidebar #370

Conversation

wbamberg commented Mar 3, 2020 • edited Loading

wbamberg commented Mar 4, 2020 • edited Loading

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe commented Mar 5, 2020

wbamberg commented Mar 6, 2020

wbamberg commented Mar 6, 2020

peterbe commented Mar 6, 2020

peterbe left a comment

Choose a reason for hiding this comment

wbamberg commented Mar 6, 2020

wbamberg commented Mar 3, 2020 •

edited

Loading

wbamberg commented Mar 4, 2020 •

edited

Loading