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

feat(macros): rewrite Learn sidebar #8132

Merged
merged 6 commits into from
Mar 4, 2023
Merged

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Feb 4, 2023

The Learn sidebar consists of two pieces:

  • a huge map of all the strings used for the titles of sidebar pages, in all the languages
  • some KS template stuff to generate the sidebar HTML

This design makes it hard to maintain:

  • every time we add new pages, all the localizers have to add the translated titles to the huge map
  • the map is so large, it's hard to orient ourselves in it
  • if pages get new titles, we have to update the titles in here or the sidebar is broken

So I thought: why not use the titles of pages for the links in the sidebar? And that's basically what this rewrite does.

Probably the biggest difference is that for "leaf pages", it does not list page names in the macro, but fetches them from the page title. This makes it a lot shorter (795 lines rather than 2334 lines) and of course makes the lists of localizable strings much shorter (each locale only has about 40 lines to worry about, rather than almost 300 lines in the old version). I'd hope that this makes it much easier to maintain for translators.

In general, it consists of three pieces:

  • the localizable strings
  • a JSON structure that describes the sidebar
  • a bit of code that turns these two pieces into HTML

It embodies a basic assumption that the sidebar has a structure like this:

  • at the top level it's a collection of sections, like "CSS - Styling the web". Each section has a heading which is a link, but the link text usually wants to be different from the page title (so needs to be listed in the macro). Each section contains one or more subsections.
  • each subsection has a heading which is not a link (so needs to be listed in the macro). For example, "CSS building blocks". Under each section is one or more links to actual pages. These always have link text matching the page title.

Overall I've tried to keep the sidebar the same, but there are a couple of differences:

  • when a page doesn't have a translation, in the current macro it does get a translated title (assuming the translated string can be found in the macro) but the link is 404. In this macro we use the smartLink() function, which if the translation does not exist, gives us the en-US title and link, and annotates the link text with [en-US]. I'm not sure if that is better or worse. For an example of what I mean, see some of the React pages in fr.
  • I couldn't figure out a sensible way to keep the external links in the "Git and GitHub" section, so I just removed them :). I don't think it makes a lot of sense to do this anyway, and we have those links in the "Git and GitHub" overview page anyway.

If we wanted to be more radical we could probably get rid of more localizable strings. For instance, we could perhaps get rid of the ones in section headings, if we wanted greater consistency between sidebar entries and page titles. Or we could use the short-title thing (#7825) to provide an alternative title (although we might want to call it alternative-title instead I suppose, since it might be longer than the page title).

(One other thing I like about this is that it clearly splits the JSON that describes the sidebar from the JS that builds it. So if something like this could be a common format for a macro (a big if) then we could imagine authors only supplying the l10n strings and the JSON, and have the JS built into Yari. This would make sidebar authoring a lot simpler and safer.)

@caugner , I'd love to know what you think. Currently this macro has merge conflicts because it was written before the ko translations, but of course it would be easy to update for them. But I don't want to do that unless we think it is worth pursuing.

@github-actions github-actions bot added macros tracking issues related to kumascript macros merge conflicts 🚧 Please rebase onto or merge the latest main. labels Feb 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Dev build for dbf1457 was deployed to: https://pr8132.content.dev.mdn.mozit.cloud/

@jasonren0403
Copy link
Contributor

jasonren0403 commented Feb 7, 2023

just passing by and noticed that there will be 3 same titles in a row.
after:
Screenshot_2023-02-07-08-50-21-645-edit_com.android.browser.jpg

before:
Screenshot_2023-02-07-08-51-10-121-edit_com.android.browser.jpg

@wbamberg
Copy link
Collaborator Author

wbamberg commented Feb 7, 2023

ugh, that is true. I suppose we could put these in separate pages, as we do for the "HTML questions" etc.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

Dev build for dbf1457 was deployed to: https://pr8132.content.dev.mdn.mozit.cloud/

@caugner
Copy link
Contributor

caugner commented Feb 7, 2023

@wbamberg FWIW Here's a diff of the current prod sidebar compared with the sidebar in this branch, with the following differences I noticed:

  • No more data-default-state="" and class="toggle" attributes (probably okay/no more needed)
  • No more direct links to https://guides.github.com/ (probably okay)
  • No more overview/ index postfixes and Assessment: prefixes (maybe these could be derived from page-types?).
  • More verbosity, e.g. Django Tutorial Part 6 instead of Tutorial Part 6 (probably okay, the "Express Tutorial" section had this before).
  • Changes in the "Common questions" section, as mentioned above (maybe the page titles could be improved, and "Howto" / "Use X to solve common problems" pages renamed to "Common questions" / "Common questions about X").
  • Three links in the "Web Forms" > "Core forms learning pathway" section without href and text undefined:
    • The HTML5 input types
    • Client-side form validation
    • Sending form data

edit: Maybe the section titles could be an additional attribute section-title on all pages that have subpages, to avoid having translations in the macro altogether?

@wbamberg
Copy link
Collaborator Author

wbamberg commented Feb 7, 2023

Thank you for this @caugner !

A lot of these are just consequences of using the page title in the sidebar, which obviously is a basic assumption of this plan. So partly we need to decide whether it's intentional for the page title and link text to diverge or not. I think usually it isn't intentional, and it's beneficial for consistency for them to be the same. I think in some cases here the sidebar link text is better and we should rename the pages.

@wbamberg FWIW Here's a diff of the current prod sidebar compared with the sidebar in this branch, with the following differences I noticed:

  • No more data-default-state="" and class="toggle" attributes (probably okay/no more needed)

Yes, I think we can remove this as we now pop open the sidebar sections in JS based on the current article.

  • No more direct links to https://guides.github.com/ (probably okay)

I'm very OK with this, in fact I think it is very jarring to have external links like this in the sidebar. In fact if you look at the page it was always the intention to have our own content for this:

Note that the links below take you to resources on external sites. Eventually, we are aiming to have our own dedicated Git/GitHub course, but for now, these will help you get to grips with the subject at hand.

(https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/GitHub#guides).

  • No more overview/ index postfixes and Assessment: prefixes (maybe these could be derived from page-types?).

If you mean, we could generate something like "Thing overview" from "Thing" and the fact that it is a learn-module-oveview page or whatever, then I think this is tricky because of l10n. This is essentially the same conversation as https://github.com/orgs/mdn/discussions/248#discussioncomment-3919029 - since Yari doesn't support localization, it's hard for us to generate titles in a way that will be robust in different locales.

My vote would be:

  • for "overview", it's fine to omit that
  • for "Assessment: ", we should retitle the pages.
  • More verbosity, e.g. Django Tutorial Part 6 instead of Tutorial Part 6 (probably okay, the "Express Tutorial" section had this before).

Yes... I think I like the less verbose option better. But as you say, we're not consistent here anyway so this doesn't look like a blocker to me. This might be a case where we use short-title?

  • Changes in the "Common questions" section, as mentioned above (maybe the page titles could be improved, and "Howto" / "Use X to solve common problems" pages renamed to "Common questions" / "Common questions about X").

Yes, maybe we could have some more consistency here.

  • Three links in the "Web Forms" > "Core forms learning pathway" section without href and text undefined:

  • The HTML5 input types

  • Client-side form validation

  • Sending form data

These are my mistake in this PR. I'll fix them now.

edit: Maybe the section titles could be an additional attribute section-title on all pages that have subpages, to avoid having translations in the macro altogether?

Sorry I don't understand this. Could you give me an example?

@wbamberg
Copy link
Collaborator Author

wbamberg commented Feb 7, 2023

  • Changes in the "Common questions" section, as mentioned above (maybe the page titles could be improved, and "Howto" / "Use X to solve common problems" pages renamed to "Common questions" / "Common questions about X").

I just filed mdn/content#24260 to try to fix this in content.

@wbamberg
Copy link
Collaborator Author

wbamberg commented Feb 8, 2023

mdn/content#24260 landed, so I just updated this PR so the "Common questions" entries get sensible text.

@caugner
Copy link
Contributor

caugner commented Feb 21, 2023

@wbamberg There is one more conflict, sorry about that.

@caugner caugner added the sidebar/toc Sidebar and table of contents label Mar 2, 2023
@caugner caugner changed the title Rewrite Learn sidebar feat(macros): rewrite Learn sidebar Mar 2, 2023
@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Mar 4, 2023
kumascript/macros/LearnSidebar.ejs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macros tracking issues related to kumascript macros sidebar/toc Sidebar and table of contents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants