-
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
chore(macros): add missing CSS selectors pages to sidebars #8014
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
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.
Thanks for your PR!
It works fine. However, I don't think we should deal with these selectors pages by creating another sub-layer of the sidebar. This is not a thing we do anywhere else in the Learn sidebar, AFAIK, or in the Learn organization.
Learn has quite a specific and usually consistent organization:
- there are top level things, like "CSS"
- then there are "modules" underneath that, like "CSS building blocks"
- then inside "modules" there is a flat list of articles
There isn't a hierarchy of articles inside "modules".
So for now at least we should list all 5 selectors articles directly under "CSS building blocks".
Really that also means we should update the "in this module" sections in all the "CSS building blocks" articles 😭 and also move these articles up a level so they are directly underneath "CSS building blocks". But let's not worry about that for now, because also:
...if we wanted to split out the CSS selectors articles into a separate group we should consider a "CSS selectors" module. And maybe we should do that, since "CSS building blocks" is pretty long now. But again that's outside the scope of this PR.
I would simply remove the "in this module" sections, because they do not provide anything, that the sidebar doesn't provide already. Alternatively one can maybe also use CSSRef macro that will be the same on all pages. |
Please don't do this. As @wbamberg pointed out, the Learning Area has a specific structure with content grouped into learning modules. The In this module is part of the structure of a Learning Area article, and we can't remove it without removing it in all LA articles, which is outside the scope of this PR. The |
Oh, I'm sorry, I meant only hypothetically, of course not in this PR. Besides, "In this module" is part of the content repository, so not in this PR anyway =) This PR, however, should already be ready for a merge. |
Sorry, my bad then. I'll leave Will to approve this PR. |
This is a very interesting point! @hamishwillee brought up the same thing in #7444 (comment). Maybe we should start a discussion on it :). I would love to be able to remove these things if we can agree that they are not needed any more. |
Yes, we should start the discussion. These "In this module" sections were necessary when the sidebars were (more) broken. They were created as a workaround and are a nightmare to maintain. If their usefulness is gone, we may remove them. (I would be happy as a these add a lot of (invalid) flaws in the flaw dashboard) |
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.
👍 thank you @drybalka ! Also, I started a discussion about removing "in this module" and people seem receptive to this idea.
Summary
Fixes mdn/content#23645
Relates to mdn/content#23814
Problem
Currently the sidebar in https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks is missing some links, for example, this one https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Selectors/Type_Class_and_ID_Selectors
Also the order of the links is wrong with respect to the content (see mdn/content#22005 )
Solution
This PR adds missing links and fixes their order.
Screenshots
Before
After