-
Notifications
You must be signed in to change notification settings - Fork 503
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): create a PWA sidebar #8238
Conversation
Thanks a lot @wbamberg. Sadly I have no had the time to go through this. |
Sorry Patrick, I forget some people don't live inside this world :). Not expanded: Expanded: The names for tutorials and tutorial steps are placeholders. One thing worth mentioning is that when an item in the sidebar has a disclosure arrow, like "Your first PWA", (meaning it contains a group of connected subpages, like steps in a tutorial) it can't also be a link to a page (because clicking the label opens the subpages), so if we want an overview page for this we usually end up having that be the first child, like:
...this is ugly and I wish we had a better answer here, but it's what we do everywhere. |
This looks great. A couple of thoughts:
|
this LGTM. I like the idea of nesting subcontent. I assume, like on https://developer.mozilla.org/en-US/docs/Learn/CSS/First_steps/Getting_started, the current section would be open and hilited, and everything else will be closed, which I think will make this nav great UX as long as we don't have every other subject listed in the nav, like the learn section does :| |
Yes - and if you try this sidebar out your should see that behavior. |
I think 15-20 is fine in a flat list, but if it looks too big we could create additional categories.
I'd envisaged this is just one page with nothing under it, linking to the reference docs that live elsewhere. We could have links to say https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API direct from the sidebar, and maybe that would be good. We get a bit into sidebar philosophy though :) of whether sidebars should contain links to pages that themselves have a different sidebar. We do this sometimes on MDN, and it works fine now but rules out certain possible futures for sidebars (see https://github.com/orgs/mdn/discussions/124#discussioncomment-4578532 for example: it makes it much harder to use lists of links in sidebars as reusable bits of IA). But like I say, we do this already sometimes, so it is certainly possible. |
kumascript/macros/PWASidebar.ejs
Outdated
output += `<li><a href="${baseURL}${item}">${await getTitle( | ||
item | ||
)}</a></li>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to extract renderLink()
, which determines both the href
and the text.
If the page does not exist in the current locale, I think we'll want to add (en-US)
behind, like web.smartlink()
already does (I don't mind not using web.smartlink()
for now, but we should extract all these helper methods somewhere before replicating them to another sidebar anyways).
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
* upstream/main: (251 commits) chore(deps-dev): bump typescript from 4.9.5 to 5.0.3 in /client/pwa (mdn#8547) chore(deps-dev): bump typescript from 4.9.5 to 5.0.3 (mdn#8543) chore(deps-dev): bump postcss-preset-env from 8.2.0 to 8.3.0 (mdn#8561) chore(deps-dev): bump @types/react from 18.0.31 to 18.0.32 (mdn#8560) chore(advertising): update copies (mdn#8562) fix(client): place hash before headings (mdn#8532) chore(deps-dev): bump ts-jest from 29.0.5 to 29.1.0 (mdn#8559) chore(deps-dev): bump black from 23.1.0 to 23.3.0 in /deployer (mdn#8555) chore(deps-dev): bump webpack-dev-server from 4.13.1 to 4.13.2 (mdn#8558) chore(deps-dev): bump swr from 2.1.1 to 2.1.2 (mdn#8556) chore(deps): bump boto3 from 1.26.99 to 1.26.104 in /deployer (mdn#8557) chore(deps-dev): bump black in /testing/integration (mdn#8554) ci(stage-build): rsync with -j html,json,txt (mdn#8549) ci(stage-build): rsync with -c (mdn#8548) chore(deps-dev): bump react-router and react-router-dom from 6.9.0 to 6.10.0 (mdn#8534) chore(deps-dev): bump @babel/preset-env from 7.20.2 to 7.21.4 (mdn#8544) chore(deps-dev): bump tailwindcss from 3.3.0 to 3.3.1 (mdn#8546) chore(deps): bump @mdn/browser-compat-data from 5.2.46 to 5.2.47 (mdn#8545) chore(deps-dev): bump @babel/core from 7.21.3 to 7.21.4 (mdn#8542) fix(docs): make icons in good/bad examples float (mdn#8531) ...
I've updated this sidebar with the new pages we've written, and to include the existing pages until we are ready to replace them all. I've also addressed review comments. Overall structure here is pretty much the same as what we had before. Details of the content changes are in mdn/content#25888. This PR needs to land with that one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just two refactoring opportunities (+ one older comment regarding the (en-US)
postfix).
kumascript/macros/PWASidebar.ejs
Outdated
output += `<li><a href="${baseURL}"><strong>${await getTitle( | ||
"" | ||
)}</strong></a></li>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract this as renderRootItem()
to describe what this does?
kumascript/macros/PWASidebar.ejs
Outdated
for (const section of sections) { | ||
output += await renderSection(section); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do the same as in line 59:
for (const section of sections) { | |
output += await renderSection(section); | |
} | |
output += (await Promise.all(sections.map(section => renderSection(section)))).join(''); |
Sorry, I forgot about the paths thing (I remembered much too late to add to this PR!). Much better this way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one trailing slash that can probably be removed.
kumascript/macros/PWASidebar.ejs
Outdated
<% | ||
|
||
const sidebar = { | ||
sidebarURL: "/docs/Web/Progressive_web_apps/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing slash:
sidebarURL: "/docs/Web/Progressive_web_apps/", | |
sidebarURL: "/docs/Web/Progressive_web_apps", |
@wbamberg Now that this PR is no longer a draft, would you mind updating the PR description to follow our template? 🙌 |
Awesome work, @captainbrosset and @wbamberg! 🎉 |
Summary
Adds a new sidebar for the PWA docs: https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps
Note: this PR needs to be merged alongside mdn/content#25888.
Problem
PWA docs don't have a usable sidebar.
Solution
This PR adds a new KS macro,
PWASidebar
, which implements a sidebar for the PWA docs, to support the PWA docs refresh project: mdn/mdn#280.The sidebar implements the information architecture discussed in the PWA planning doc: https://docs.google.com/document/d/1-skaKXoJ0rKhgBHchTWSTAW5dxrlmVYkPeKUWpet9nE/edit?usp=sharing.
The sidebar is implemented as a model of how we could have JSON-driven sidebars, with a JSON structure defining the sidebar in mdn/content and a little code in Yari that builds a sidebar from that JSON. This would allow us to retire most of the KS sidebar macros (a few complex sidebars like DefaultAPISidebar would need to stay in KS for the time being).
Screenshots
Before
After
How did you test this change?
CONTENT_ROOT
: https://github.com/mdn/yari/blob/main/docs/REVIEWING.mdyarn dev