Repository navigation
fix(theme-classic): render mobile HTML navbar items as list items - #12787
arushii-codes wants to merge 1 commit into
Conversation
|
Hi @arushii-codes! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
10k11
left a comment
There was a problem hiding this comment.
Reviewed and checked locally (built the packages, ran the PR's test file with vitest: 3/3 pass; ESLint reports nothing for it).
The fix is correct. NavbarMobilePrimaryMenu renders every item inside <ul className="menu__list">, so HtmlNavbarItem must emit an <li> when mobile is set. The menu__list-item class was already applied for mobile; only the tag was wrong. Dropdown items already used li, so behaviour there is unchanged.
Small, non-blocking suggestions:
- Stray blank line. The added empty line after
const Comp = ...inHtmlNavbarItem.tsxis unrelated to the change; dropping it keeps the diff to one line. - Test strength. The mobile test checks the tag only. Since the bug was an invalid
divinside aul, rendering the item inside a<ul className="menu__list">and also asserting themenu__list-itemclass (and absence ofnavbar__item) would pin the real contract, so a future change of class logic is caught as well. - The
screen.getByRole('link', ...)check is a nice way to show the HTML is still injected.renderis not auto-cleaned in this vitest setup, so if morescreenqueries are added later, callcleanup()or use thecontainerqueries to avoid cross-test matches.
FYI this overlaps with #12806, which makes the identical one-line change for #12578, so the maintainers may want to pick one.
Pre-flight checklist
Motivation
Test Plan
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
Summary by CodeRabbit