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.

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

Related content experiment #3928

Closed
wants to merge 19 commits into from
Closed

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented May 28, 2021

Early days!

What this does is that it moves from a kumascript/macros/*.ejs to a build/related-content/*.js file.
Basically, instead of a EJS macro that generates a blob of HTML, it's a plain Node function that generates an array. The array is meant to be "infinitely recursive". But it basically looks like this:

[{title: "Heading", url: "/en-US/docs/Foo", content: [
  {title: "Sub-heading", url: "/en-US/docs/Foo/page", content: [
    // ...
  ]
]

The killer feature of this is that on the React side, the HTML that is generated is done in a way that it automatically expanded the <details> tags based on the current page you're on might be inside its <details><ol> tag. All others are closed.

The other big difference is that Node is easier to debug and reason about. E.g. debugging. ...than KS EJS files.

Since the sidebarHTML can get large and that it needs to be encoded as a serialized JSON string too, for hydration, we stand to make the sidebar payload much smaller. For server-side rendering, we only need to render the HTML elements that will be visible. E.g. the CSS sidebars are ~70KB of HTML, but if you delete all the HTML that you can't see anyway, the size becomes 5KB. The list of URLs and titles aren't going away but the SSR HTML will be potentially much smaller.

In summary

The benefits of this are:

  1. The current page you're on is automatically highlighted. It figures out which leaf node contains the current active page and makes that one open by default. All other are collapsed.
  2. We draw the link titles from the documents instead of duplicating the title inside the KS code.
  3. Because it checks each and every link, pages that are missing become red. And for translated-content, if the page isn't available, it automatically falls back to the en-US URL.
  4. Because it only needs to server-side render the leaf nodes that should be expanded, all that's visually hidden is excluded from the HTML. In one example, the HTML, just for the sidebar, goes from 28K to 4.3K.
  5. Build-times are ~20% faster.

@peterbe
Copy link
Contributor Author

peterbe commented May 28, 2021

@hamishwillee you'll love this. :)

@peterbe
Copy link
Contributor Author

peterbe commented May 28, 2021

One thought I had was...
There's no standard sidebar for the Glossary pages. A bunch of them have a custom little hardcoded one.
E.g. https://github.com/mdn/content/blob/2d7722a59a0c07fde6197f340a845e625faef175/files/en-us/glossary/adobe_flash/index.html#L13-L29
But if you look carefully, a lot of those links don't belong as a sidebar. They should be morphed into a "See also" section, below the regular content.

With this branch here, we could write something like:

if (doc.slug.startsWith("Glossary") {
  doc.related_content = getRelatedContent("glossary", doc);
}

and then we build a fresh new (in pure Node) custom sidebar for ALL Glossary pages. Perhaps a getSubpagesExpand() like kumascript does but grouped nicely by the first character of the name.

@hamishwillee
Copy link
Contributor

Yes I do love this :-).

As an aside, the JSON is still manually crafted. Two things it may be useful to think about at this point

  • What if the current page is not specified in a sidebar. Can/should we auto generate a sidebar based on peers or closest parent?
  • Where not specified, can we auto specify based on file structure.
  • Essentially I think every doc should have a home in a sidebar but sometimes you don't care too much about the order. Being able to say "expand below this point automatically" can be handy.

I'm not entirely sure what the proposed glossary thing will look like. It is worth a discussion, but my thinking is that what I might like to see on a glossary is thee sections:

  • All glossary items
  • Manually crafted see also related links like those quicklinks.
  • The topics that link to this glossary item

Anyway, this is going to be a much better infrastructure to build on than what we have. Can't wait to play with it.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 3, 2021

One of the key benefits of this is that we can ship a LOT less in the server-side rendered HTML if we know a certain <details> block isn't expanded. When the client-side rendered takes over, it makes sure they're all there. That's part of the React hydration. Right now, the JSX logic is like this:

<details open={node.containsActive}>
  {(!isServer || node.containsActive) && <ol>...</ol>}
</details>

What that means is that the SSR HTML only contains the HTML that is expanded.

I tested this; the HTML of the whole <nav id="sidebar-quicklinks" class="sidebar">... in a client-side rendering is 28K. But the HTML shipped from the server is only 4.3K. That has a huge potential for much better web performance.
Now, this only tackles the Learn/* and MDN/* slugs but imagine we could do this for the Web/CSS/* trees. There, today, the total HTML is 33K.

The visual difference is none. If you were to consume these pages without JavaScript enabled, (i.e. no React hydration) you wouldn't be able to expand any of the <details> tags.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 3, 2021

@hamishwillee @wbamberg @Elchi3 I need your help. I need your help in shipping this into production.

So far this has been a pseudo-experiment and I've chosen to only tackle MDNSidebar and LearnSidebar. I picked these because they were simple to work with. They don't contain any complicated JS that dynamically figures out which links to include. And I'd like to keep it like that and tackle other, more complex, sidebars later.

But I'm running out of time here. I need to focus on the MDN Plus stuff.

I don't worry about getting the tests to pass. What I need help with is testing and sanity checking.
Can you check out the branch and try it out. Click around and make sure it works perfectly. We can rope in @schalkneethling to help with some styling because I think the margins and paddings are different now. But that feels less important because I think the sidebars are now significantly better because they can A) highlight the page you're on, B) make the web pages faster for initial load, C) the titles aren't duplicated.

There's some interesting business logic to discuss too. Look at what I did in build/index.js where I "assume" to override whatever KS sidebar is used and automatically switch to this new related_content implementation.

Another area where we should discuss is the grabbing of titles for the links. In the old KS sidebars each title is determined by the massive hash table of all tables. See this code for example.
In my implementation, I ignore those and instead pick up the link texts automatically from the document itself. I left a couple where I noticed the sidebar's preferred link text is different from the document's title.
One thing we can consider is to introduce a new short-title: key in the front-matter that gets used (if set) in the sidebar.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 3, 2021

It's less important but still somewhat important. The build-times are roughly 20% faster now (for the Learn/* tree at least). This new implementation is faster because it's probably more efficient to do it in pure Node as opposed to kumascript.

@hamishwillee
Copy link
Contributor

@peterbe I finally managed to get this working in a Ubuntu VM but I've run out of time to properly test it due to my "real work".

My problem is I'm not 100% sure what I should be seeing vs not seeing. I think this should just be affecting the related topics section of sidebar as injected by {{LearnSidebar}}

It seems to be working fine with most of the same behaviour as the current sidebar - sidebar opens to the current page when you load it. All other trees are closed at this point.

Looking at http://localhost:3000/en-US/docs/Learn/Server-side/Express_Nodejs/mongoose#setting_up_the_mongodb_database I do see that the link looks like this - italic and clickable. It is highlighted but I would make it bold and non-clickable for current page. Try it - you'll see that you get taken to the top of the page so the sidebar context is lost.

image

With respect to your discussion about sidebars and business logic, I am not so confident that I can add much useful feedback.

But here goes "my 2 bits":

  • I think of sidebars as content. That means I would prefer them as part of content definition than in yari.

  • I have seen sidebar structures generated entirely from metadata, e.g. using fields for relative page order and title in front metadata. I have also used fully hard coded sidebars as we do now.

    • I have a sneaky liking for getting the title from a metadata field but my my general feeling is that it is much easier to plan a sidebar if you can see the whole structure. For example, you can easily see the things you don't have a title for yet.
    • If you get sidebar page title from a field we will have to make sure that we don't swamp it from the US version, except if it isn't defined.
    • Upshot, just not sure of "what is best"
  • Re

    There's some interesting business logic to discuss too. Look at what I did in build/index.js where I "assume" to override whatever KS sidebar is used and automatically switch to this new related_content implementation.

    I don't know what this means. Just to be clear though, we need to keep the other things in the sidebar like those injected by quick links.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 4, 2021

My problem is I'm not 100% sure what I should be seeing vs not seeing. I think this should just be affecting the related topics section of sidebar as injected by {{LearnSidebar}}

"Related topics" is just the headline for all sidebars.
What you should be seeing is that most of the en-US/docs/Learn pages should have a new sidebar. One that highlights the page you're on and collapses all other nodes.
Yes, a page that uses to use {{LearnSidebar}} now gets its sidebar from an entirely different implementation.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 4, 2021

* > There's some interesting business logic to discuss too. Look at what I did in build/index.js where I "assume" to override whatever KS sidebar is used and automatically switch to this new related_content implementation.
  
  
  I don't know what this means. Just to be clear though, we need to keep the other things in the sidebar like those injected by quick links.

There are other pages that didn't use {{LearnSidebar}}. Either they had no sidebar at all or they used some other solution to (KS or hardcoded) to make a sidebar. All of these are unaffected. My solution only covers documents that use {{LearnSidebar}}.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 4, 2021

* I think of sidebars as **content**. That means I would prefer them as part of content definition than in yari.

I don't disagree but it quickly gets hairy to debate that. The Learn sidebar and the MDN sidebar are trivial.
Right now, instead of blocks of Node code, they could be Yaml that is read from the mdn/content repo. E.g.

/
  /.github
  /files
  /sidebars
    /learn.yml
    /mdn.yml
    ...
  /README.md
  ...

but there are plenty of sidebars that sufficiently complicated that they quickly "outgrow" Yaml. E.g. the clustering by leading character for SVG sidebars. E.g. https://developer.mozilla.org/en-US/docs/Web/SVG/Element/altGlyph
Or CSS sidebars that rely on tags. Or sidebars that rely on mdn-data.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 4, 2021

* If you get sidebar page title from a field we will have to make sure that we don't swamp it from the US version, except if it isn't defined.

I don't understand this sentence.

@hamishwillee
Copy link
Contributor

Thanks, I will test again on Monday.

Re "I don't understand this sentence". Yoiks, sorry. All I meant here is that there is work you guys are doing to make sure US doc-version tags and browser-compat keys etc get used in localisations. We wouldn't want all keys to be affected by that change though, if they actually are locale specific.

@hamishwillee
Copy link
Contributor

hamishwillee commented Jun 6, 2021

@peterbe Thanks for the explanations. I have had a further play. I wouln't say "robustly" but hit various links and open closed pages for 20 mins. It seems very much the same as currently - certainly "no worse". It is hard to say that it is better because I already find it pretty fast and things like a lower transmitted data size don't really affect me for local testing. But a better underlying infrastructure for these things is a win even if it doesn't change much for user facing stuff.

Reiterating the problem with link behaviour: italic highlighting is hard to see, particularly if the link is off screen and you're searching for it in a sea of other links. Further, if you click that link the new page is loaded and the link disappears. Unless you add support to scroll to the current sidebar open link, the sidebar link should not be clickable.

image

Essentially though it is no worse from an end user point of view so if no one else runs into problems during testing sounds like a good move.

FMI, because this is what I see as the next step ... does the new definition allow us to collapse the top level? Specifically, I want to be able to make that learn sidebar into around 7 top level twisties
image

@dalexsy
Copy link
Contributor

dalexsy commented Jun 7, 2021

On hold indefinitely

@dalexsy dalexsy closed this Jun 7, 2021
@escattone
Copy link
Contributor

@dalexsy It's not your place to announce a decision on this and close it. First you need to understand it fully, the reason for it, and then make an argument for why it should be "on hold". This is the first moment I've had a chance to understand this in more detail, and the thing that strikes me is that this is essentially a refactoring (for speed, ease of maintenance, and reliability) of the way we generate sidebars -- simplifying that functionality by moving that functionality out of the macros and into a single function within the builder -- and doesn't preclude any future UX plans around sidebars, but in fact makes them easier to do when that time comes.

@escattone escattone reopened this Jun 7, 2021
@dalexsy
Copy link
Contributor

dalexsy commented Jun 8, 2021

@escattone I disagree with you there. I shared with the rest of the team that I have a plan for how we would restructure the sidebar as part of the redesign, and it was my intention that we would be able to work on this together as a team. My concern is if you all continue working on this now without my input, we're going to have to be redoing work down the line when I do have the capacity to be involved.

@HerminaC
Copy link
Contributor

HerminaC commented Jun 8, 2021

@escattone: taking this conversation to a private thread with everyone involved. This type of conversation on a public repo can only confuse the community and hurt the team - i would appreciate if you could avoid it in the future. Also, even if this is now reopened, please put on hold any work on it until we have clarified things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants