Repository navigation
fix(theme-classic): render mobile HTML navbar items as list items - #12806
khushalrajput45 wants to merge 1 commit into
Conversation
|
Hi @khushalrajput45! 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 cla@meta.com. 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 and minimal. The mobile menu (NavbarMobilePrimaryMenu) renders items inside <ul className="menu__list">, so an HTML item there must be an <li>; mobile || isDropdownItem ? 'li' : 'div' fixes #12578 without touching the desktop path, and the three tests cover desktop, mobile and dropdown.
Non-blocking suggestions:
- License header. The new test file has no
Copyright (c) Facebook, Inc. and its affiliates.header, unlike the neighbouring test files (e.g.theme/Tabs/__tests__/index.test.tsx). Lint doesn't flag it for me, but it's the repo convention. - Test strength. The mobile test checks the tag only. Rendering the item inside a
<ul className="menu__list">and asserting themenu__list-itemclass (and nonavbar__item) would pin down the real contract, not just the tag. - Overlap: #12787 opened earlier for the same issue with the identical source change, so the maintainers may want to merge only one.
Description
Fixes #12578.
HTML navbar items currently render as
<div>elements on mobile, whereas the mobile navigation list expects<li>elements.This change renders HTML navbar items as
<li>elements on mobile while preserving the existing behavior for desktop navbar items and dropdown items.Adds regression tests covering mobile, desktop, and dropdown rendering.
Testing
<li><div><li>Ran:
All 3 tests pass locally.
Summary by CodeRabbit