-
Notifications
You must be signed in to change notification settings - Fork 509
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
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
Dev build for dbf1457 was deployed to: https://pr8132.content.dev.mdn.mozit.cloud/ |
ugh, that is true. I suppose we could put these in separate pages, as we do for the "HTML questions" etc. |
Dev build for dbf1457 was deployed to: https://pr8132.content.dev.mdn.mozit.cloud/ |
@wbamberg FWIW Here's a diff of the current prod sidebar compared with the sidebar in this branch, with the following differences I noticed:
edit: Maybe the section titles could be an additional attribute |
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.
Yes, I think we can remove this as we now pop open the sidebar sections in JS based on the current article.
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:
(https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/GitHub#guides).
If you mean, we could generate something like "Thing overview" from "Thing" and the fact that it is a My vote would be:
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
Yes, maybe we could have some more consistency here.
These are my mistake in this PR. I'll fix them now.
Sorry I don't understand this. Could you give me an example? |
I just filed mdn/content#24260 to try to fix this in content. |
mdn/content#24260 landed, so I just updated this PR so the "Common questions" entries get sensible text. |
@wbamberg There is one more conflict, sorry about that. |
The Learn sidebar consists of two pieces:
This design makes it hard to maintain:
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:
It embodies a basic assumption that the sidebar has a structure like this:
Overall I've tried to keep the sidebar the same, but there are a couple of differences:
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.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 italternative-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.