Issues/314 enhanchment title unify header#1048
Open
sanjayahari wants to merge 13 commits intoopenwisp:masterfrom
Open
Issues/314 enhanchment title unify header#1048sanjayahari wants to merge 13 commits intoopenwisp:masterfrom
sanjayahari wants to merge 13 commits intoopenwisp:masterfrom
Conversation
…n-pages.Unified the rendering logic for desktop and mobile navigation, logos and language switcher. Instead of maintaining two separate link-generating loops, both now utilize the renderNavLinks helper method.Added test cases. openwisp#314
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHeader was refactored to expose three public render helpers: renderLogos(deviceClass), renderLanguageSwitcher(deviceClass), and renderNavLinks(deviceClass). The main render delegates to these helpers and uses a unified-header structure with device-class-aware flows. Mobile hamburger/menu handling and a mobile menu container were reworked to use the new helpers. Unit tests were added to verify active-link application, logo rendering, link and language-button counts, and mobile menu toggle behavior. Extensive responsive and structural header CSS was added. package.json Jest transformIgnorePatterns was updated to also allow transforming cheerio. needs-verify removed the user.method === "bank_card" check for the bank_card branch. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/components/header/header.js`:
- Line 1: The file header.js contains a leading BOM character before the comment
"/* eslint-disable react/no-array-index-key */"; remove the invisible Byte Order
Mark at the very start of the file so the file begins directly with the eslint
comment (no other code changes), then save/commit to ensure
bundlers/linters/diff tools no longer see the BOM.
- Around line 120-128: The internalLinks comparison is using resolved URLs while
link.url still contains the "{orgSlug}" placeholder, so
internalLinks.indexOf(link.url) will always be -1; fix the isInternal
calculation in the header component by comparing against the resolved URL (e.g.,
compute const resolvedUrl = link.url.replace("{orgSlug}", orgSlug)) and use
internalLinks.indexOf(resolvedUrl) (and for pathname equality compare pathname
to resolvedUrl) so functions/vars to update are the isInternal assignment, the
activeClass pathname check, and references to link.url in this block (helpers:
isInternalLink, shouldLinkBeShown, internalLinks).
In `@client/components/header/header.test.js`:
- Around line 279-292: Replace the deprecated Enzyme call .filterWhere with
.filter in the test: update the two lines that create loginLink and signupLink
(the wrapper.find('.header-link').filterWhere(...) expressions) to use
wrapper.find('.header-link').filter(predicate) instead, keeping the same
predicate logic (n => n.prop('to') === '/default/login' and n => n.prop('to')
=== '/default/registration') so loginLink and signupLink creation uses .filter
rather than .filterWhere.
In `@client/components/header/index.css`:
- Around line 96-98: There are two duplicate CSS rules for the selector
.header-link.active; remove the redundant one and consolidate into a single
definition (keep the more complete rule currently at the later occurrence) so
all properties for .header-link.active live in one place; update any differing
declarations by merging their properties into the remaining .header-link.active
rule and delete the earlier/duplicate block to avoid specificity/confusion.
- Line 145: Consolidate the two mobile media-query blocks into a single block by
merging the styles from the duplicate "@media screen and (min-width: 0px) and
(max-width: 767px)" and the other "@media ... (max-width: 767px)" into one
"@media (max-width: 767px)" section; remove the redundant "min-width: 0px",
ensure rules for selectors in header styles (e.g., any rules currently inside
those media queries such as .header, .nav, .mobile-menu) are combined without
duplication and that ordering/ specificity is preserved so mobile styles apply
consistently and are easier to maintain.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.csspackage.json
📜 Review details
🔇 Additional comments (6)
client/components/header/header.test.js (1)
294-313: New tests for unified layout rendering look good.These tests validate the unified header behavior including logo rendering, link counts, language button counts, and mobile menu toggle functionality.
client/components/header/header.js (3)
64-94: ModularrenderLogoshelper looks good.Clean extraction of logo rendering logic with proper optional chaining for conditional rendering.
96-113: ModularrenderLanguageSwitcherhelper looks good.Clean implementation with appropriate class composition for device-specific styling.
148-199: Render method structure is well-organized.The unified header layout with separate rows for logos, navigation, and mobile menu is cleanly structured with appropriate responsive class handling.
client/components/header/index.css (1)
248-291: New responsive utility classes and unified header styling look good.The addition of
.mobile-only,.desktop-only,.unified-header, and related responsive rules provides a solid foundation for the unified header layout.package.json (1)
107-109:cross-envusage is incorrect and may be missing from dependencies.Two issues:
Missing dependency:
cross-envis not declared independenciesordevDependencies(if this is true, these scripts will fail during build).Incorrect syntax: Using
&&aftercross-envbreaks environment variable propagation.cross-envsets variables only for the command directly following it. With&&, subsequent commands (likenpm run setupandwebpack) will not receive theNODE_OPTIONSvariable.Correct approaches:
- Apply
cross-envonly to the command that needs the variable:- "build": "cross-env NODE_OPTIONS=--openssl-legacy-provider && npm run setup && webpack --mode production --progress --config config/webpack.config.js", + "build": "npm run setup && cross-env NODE_OPTIONS=--openssl-legacy-provider webpack --mode production --progress --config config/webpack.config.js",
- Or use
cross-env-shellfor inline shell commands with proper quoting.If
cross-envis not in devDependencies, add it:"devDependencies": { + "cross-env": "^7.0.3",
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 107-109: package.json scripts "build", "build-dev", and "client"
misuse cross-env (and cross-env is missing from devDependencies): add
"cross-env" to devDependencies and change each script so cross-env prefixes the
webpack command that needs NODE_OPTIONS rather than being a separate command;
for example, keep running setup first (npm run setup) then invoke
webpack/webpack serve with cross-env NODE_OPTIONS=--openssl-legacy-provider ...
so the environment variable is applied to the webpack/serve process (update
script names build, build-dev, client accordingly).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
…n-pages.Unified the rendering logic for desktop and mobile navigation, logos and language switcher. Instead of maintaining two separate link-generating loops, both now utilize the renderNavLinks helper method.Added test cases.Resolved some checks openwisp#314
…n-pages.Unified the rendering logic for desktop and mobile navigation, logos and language switcher. Instead of maintaining two separate link-generating loops, both now utilize the renderNavLinks helper method.Added test cases.Resolved some checks.Resolved yarn.lock merge conflict openwisp#314
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/components/header/index.css (1)
52-61:⚠️ Potential issue | 🟠 MajorDo not remove keyboard focus visibility on language buttons.
outline: noneis set, but there is no replacement focus style for keyboard navigation.Proposed fix
.header-language-btn { @@ - outline: none; cursor: pointer; } + +.header-language-btn:focus-visible { + outline: 2px solid `#d23c22`; + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/index.css` around lines 52 - 61, The .header-language-btn rule removes keyboard focus visibility; revert/remove outline: none and add an accessible focus indicator by defining a :focus and/or :focus-visible style for .header-language-btn (e.g., visible outline or box-shadow with sufficient contrast) so keyboard users can see focus when tabbing; ensure the new focus style is non-destructive to layout and meets contrast/visibility requirements.
♻️ Duplicate comments (5)
client/components/header/index.css (2)
96-98: 🧹 Nitpick | 🔵 TrivialConsolidate duplicate
.header-link.activedeclarations.This selector is defined twice; keep one merged block to avoid drift and confusion.
Also applies to: 138-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/index.css` around lines 96 - 98, Duplicate CSS rule for the selector .header-link.active appears in the stylesheet; consolidate by keeping a single .header-link.active rule and merge any differing properties from the two blocks into that one declaration, then remove the other duplicate to prevent drift and confusion (ensure any styles from both occurrences are preserved in the remaining .header-link.active rule).
145-145: 🧹 Nitpick | 🔵 TrivialMerge overlapping mobile media-query blocks for the same breakpoint.
You currently maintain two
max-width: 767pxblocks; consolidate into one block for predictable ordering and simpler maintenance.Also applies to: 266-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/index.css` at line 145, There are duplicate mobile media-query blocks using "@media screen and (min-width: 0px) and (max-width: 767px)"; locate both blocks (one starting around the existing selector at line ~145 and the other at ~266-289) and consolidate them into a single `@media` block named the same, moving all contained rules into that single block, resolving any conflicting declarations by preserving the intended final value (last-wins) and keeping selector order; remove the duplicate block afterwards so there is only one mobile media-query for that breakpoint to simplify maintenance.client/components/header/header.js (2)
126-130:⚠️ Potential issue | 🟠 MajorFix internal-link comparison to use resolved URL.
internalLinkscontains resolved paths, but comparison uses rawlink.urlwith{orgSlug}placeholder, so matching is unreliable.Proposed fix
{links?.map((link, index) => { if (!shouldLinkBeShown(link, isAuthenticated, userData)) return null; + const resolvedUrl = link.url.replace("{orgSlug}", orgSlug); const isInternal = isInternalLink(link.url) && - (internalLinks.indexOf(link.url) < 0 || !isAuthenticated); + (internalLinks.indexOf(resolvedUrl) < 0 || !isAuthenticated); const activeClass = - pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : ""; + pathname === resolvedUrl ? "active" : ""; @@ - <Link {...commonProps} to={link.url.replace("{orgSlug}", orgSlug)}> + <Link {...commonProps} to={resolvedUrl}>Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` around lines 126 - 130, The internal-link check and activeClass comparison are using the raw link.url (which may contain the "{orgSlug}" placeholder) instead of the resolved path; update the logic in isInternal (and the activeClass computation) to first resolve link.url by replacing "{orgSlug}" with orgSlug (e.g., resolvedUrl = link.url.replace("{orgSlug}", orgSlug)) and then use resolvedUrl when checking internalLinks.indexOf(...) and when comparing to pathname; reference symbols: isInternalLink, internalLinks, link.url, orgSlug, isInternal, activeClass, pathname.
1-1:⚠️ Potential issue | 🟡 MinorRemove the BOM at file start.
The leading BOM triggers lint/tooling issues and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` at line 1, The file header.js contains a leading Byte Order Mark (BOM) before the eslint-disable comment which triggers lint/tooling issues; open header.js and re-save/remove the BOM so the first character is the slash of "/* eslint-disable react/no-array-index-key */" (UTF-8 without BOM) ensuring no invisible characters precede that comment.package.json (1)
108-110:⚠️ Potential issue | 🔴 CriticalFix
cross-envusage in build scripts (currently ineffective) and declare it.
cross-envis being run as a standalone command, soNODE_OPTIONSis not applied towebpack, andcross-envis not declared indevDependencies. This can break build/dev scripts.Proposed fix
"devDependencies": { + "cross-env": "^7.0.3", "@babel/core": "^7.28.5", "@babel/eslint-parser": "^7.28.5", "@babel/helper-function-name": "^7.24.7", @@ - "build": "cross-env NODE_OPTIONS=--openssl-legacy-provider && npm run setup && webpack --mode production --progress --config config/webpack.config.js", - "build-dev": "cross-env NODE_OPTIONS=--openssl-legacy-provider && npm run setup && webpack --mode development --progress --config config/webpack.config.js", - "client": "cross-env NODE_OPTIONS=--openssl-legacy-provider && npm run setup && webpack serve --host 0.0.0.0 --mode development --config config/webpack.config.js", + "build": "npm run setup && cross-env NODE_OPTIONS=--openssl-legacy-provider webpack --mode production --progress --config config/webpack.config.js", + "build-dev": "npm run setup && cross-env NODE_OPTIONS=--openssl-legacy-provider webpack --mode development --progress --config config/webpack.config.js", + "client": "npm run setup && cross-env NODE_OPTIONS=--openssl-legacy-provider webpack serve --host 0.0.0.0 --mode development --config config/webpack.config.js",#!/bin/bash set -euo pipefail echo "Checking cross-env declaration:" rg -n '"cross-env"' package.json echo echo "Checking build script forms:" rg -n '"build"|"build-dev"|"client"' package.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 108 - 110, The build scripts ("build", "build-dev", "client") misuse cross-env by running it as a standalone command so NODE_OPTIONS is not applied to webpack; update those scripts to run the whole chained command under cross-env (e.g., use cross-env-shell to wrap "NODE_OPTIONS=... npm run setup && webpack ..." or ensure cross-env prefixes the webpack invocation) and add "cross-env" to devDependencies in package.json so it's installed; target the "build", "build-dev", "client" script entries and the devDependencies section when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/components/header/header.js`:
- Around line 81-85: The header image render currently only checks secondLogo
which can still call getAssetPath(orgSlug, secondLogo.url) with an undefined
url; update the guard to verify the URL exists (e.g., change the conditional
around the block that renders the <img> to secondLogo && secondLogo.url) so the
image and getAssetPath are only invoked when secondLogo.url is present,
referencing the header component's secondLogo usage and getAssetPath call to
locate the code.
In `@client/components/header/header.test.js`:
- Around line 287-295: The tests currently call .first().hasClass("active") on
the Enzyme wrappers loginLink and signupLink, which only checks the first match
and can miss regressions in mobile/desktop duplicates; update the assertions to
validate all matched nodes by using Enzyme's everyWhere (or an equivalent
iteration) on loginLink and signupLink so that loginLink.everyWhere(node =>
node.hasClass("active")) is true and signupLink.everyWhere(node =>
!node.hasClass("active")) is true, replacing the .first() assertions for both
cases.
In `@client/components/header/index.css`:
- Around line 122-127: The .sticky-msg CSS rule uses the deprecated property
word-wrap; replace it with overflow-wrap: break-word to satisfy stylelint and
modern CSS standards—update the .sticky-msg block to remove word-wrap and add
overflow-wrap: break-word while keeping the existing margin-left, width,
text-align, and line-height declarations.
---
Outside diff comments:
In `@client/components/header/index.css`:
- Around line 52-61: The .header-language-btn rule removes keyboard focus
visibility; revert/remove outline: none and add an accessible focus indicator by
defining a :focus and/or :focus-visible style for .header-language-btn (e.g.,
visible outline or box-shadow with sufficient contrast) so keyboard users can
see focus when tabbing; ensure the new focus style is non-destructive to layout
and meets contrast/visibility requirements.
---
Duplicate comments:
In `@client/components/header/header.js`:
- Around line 126-130: The internal-link check and activeClass comparison are
using the raw link.url (which may contain the "{orgSlug}" placeholder) instead
of the resolved path; update the logic in isInternal (and the activeClass
computation) to first resolve link.url by replacing "{orgSlug}" with orgSlug
(e.g., resolvedUrl = link.url.replace("{orgSlug}", orgSlug)) and then use
resolvedUrl when checking internalLinks.indexOf(...) and when comparing to
pathname; reference symbols: isInternalLink, internalLinks, link.url, orgSlug,
isInternal, activeClass, pathname.
- Line 1: The file header.js contains a leading Byte Order Mark (BOM) before the
eslint-disable comment which triggers lint/tooling issues; open header.js and
re-save/remove the BOM so the first character is the slash of "/* eslint-disable
react/no-array-index-key */" (UTF-8 without BOM) ensuring no invisible
characters precede that comment.
In `@client/components/header/index.css`:
- Around line 96-98: Duplicate CSS rule for the selector .header-link.active
appears in the stylesheet; consolidate by keeping a single .header-link.active
rule and merge any differing properties from the two blocks into that one
declaration, then remove the other duplicate to prevent drift and confusion
(ensure any styles from both occurrences are preserved in the remaining
.header-link.active rule).
- Line 145: There are duplicate mobile media-query blocks using "@media screen
and (min-width: 0px) and (max-width: 767px)"; locate both blocks (one starting
around the existing selector at line ~145 and the other at ~266-289) and
consolidate them into a single `@media` block named the same, moving all contained
rules into that single block, resolving any conflicting declarations by
preserving the intended final value (last-wins) and keeping selector order;
remove the duplicate block afterwards so there is only one mobile media-query
for that breakpoint to simplify maintenance.
In `@package.json`:
- Around line 108-110: The build scripts ("build", "build-dev", "client") misuse
cross-env by running it as a standalone command so NODE_OPTIONS is not applied
to webpack; update those scripts to run the whole chained command under
cross-env (e.g., use cross-env-shell to wrap "NODE_OPTIONS=... npm run setup &&
webpack ..." or ensure cross-env prefixes the webpack invocation) and add
"cross-env" to devDependencies in package.json so it's installed; target the
"build", "build-dev", "client" script entries and the devDependencies section
when applying the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.cssclient/utils/needs-verify.jspackage.json
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (4)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/utils/should-link-be-shown.js (2)
shouldLinkBeShown(2-29)userData(3-3)client/utils/check-internal-links.js (1)
isInternalLink(1-4)client/utils/get-text.js (1)
getText(1-5)
🪛 ESLint
client/components/header/header.js
[error] 1-1: Unexpected Unicode BOM (Byte Order Mark).
(unicode-bom)
🪛 Stylelint (17.3.0)
client/components/header/index.css
[error] 126-126: Expected "word-wrap" to be "overflow-wrap" (property-no-deprecated)
(property-no-deprecated)
🔇 Additional comments (2)
client/utils/needs-verify.js (1)
21-23: Bank-card verification gating looks correct.The added checks for payment URL and subscriptions are appropriate for this path.
client/components/header/header.test.js (1)
310-321: Good interaction coverage for hamburger/mobile menu state transition.This test directly validates class toggling tied to
menustate and strengthens regression safety.
sanjayahari
force-pushed
the
issues/314-enhanchment-title-unify-header
branch
from
2baa57e to
59b2ee1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
client/components/header/header.js (2)
81-85:⚠️ Potential issue | 🟡 MinorGuard second logo rendering by URL presence.
secondLogotruthiness alone is insufficient;getAssetPath(orgSlug, secondLogo.url)still runs with an undefined URL.Proposed fix
- {secondLogo && ( + {secondLogo?.url && ( <div className="header-logo-2"> <img src={getAssetPath(orgSlug, secondLogo.url)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` around lines 81 - 85, The conditional rendering currently checks only secondLogo, but still calls getAssetPath(orgSlug, secondLogo.url) which can run with an undefined URL; update the guard to require a URL (e.g., check secondLogo && secondLogo.url or use optional chaining secondLogo?.url) before rendering the <div className="header-logo-2"> and before invoking getAssetPath, and provide a safe alt/fallback value for secondLogo.alternate_text to avoid accessing undefined properties.
126-130:⚠️ Potential issue | 🟠 MajorFix internal-link detection to compare resolved URLs.
internalLinksstores resolved paths, but the check uses templatedlink.url, so the condition is ineffective.Proposed fix
- const isInternal = - isInternalLink(link.url) && - (internalLinks.indexOf(link.url) < 0 || !isAuthenticated); - const activeClass = - pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : ""; + const resolvedUrl = link.url.replace("{orgSlug}", orgSlug); + const isInternal = + isInternalLink(link.url) && + (internalLinks.indexOf(resolvedUrl) < 0 || !isAuthenticated); + const activeClass = pathname === resolvedUrl ? "active" : ""; @@ - <Link {...commonProps} to={link.url.replace("{orgSlug}", orgSlug)}> + <Link {...commonProps} to={resolvedUrl}>Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` around lines 126 - 130, The internal-link check uses the templated link.url while internalLinks contains resolved paths; compute a resolvedUrl once (e.g. const resolvedUrl = link.url.replace("{orgSlug}", orgSlug)) and use resolvedUrl in both the isInternalLink/internalLinks membership check and when comparing to pathname (replace usages of link.url in the isInternal/internalLinks.indexOf and activeClass comparison with resolvedUrl); apply the same change to the similar occurrence around the other check at line ~137.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/components/header/header.js`:
- Around line 81-85: The conditional rendering currently checks only secondLogo,
but still calls getAssetPath(orgSlug, secondLogo.url) which can run with an
undefined URL; update the guard to require a URL (e.g., check secondLogo &&
secondLogo.url or use optional chaining secondLogo?.url) before rendering the
<div className="header-logo-2"> and before invoking getAssetPath, and provide a
safe alt/fallback value for secondLogo.alternate_text to avoid accessing
undefined properties.
- Around line 126-130: The internal-link check uses the templated link.url while
internalLinks contains resolved paths; compute a resolvedUrl once (e.g. const
resolvedUrl = link.url.replace("{orgSlug}", orgSlug)) and use resolvedUrl in
both the isInternalLink/internalLinks membership check and when comparing to
pathname (replace usages of link.url in the isInternal/internalLinks.indexOf and
activeClass comparison with resolvedUrl); apply the same change to the similar
occurrence around the other check at line ~137.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
client/components/header/header.jspackage.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (4)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/utils/should-link-be-shown.js (2)
shouldLinkBeShown(2-29)userData(3-3)client/utils/check-internal-links.js (1)
isInternalLink(1-4)client/utils/get-text.js (1)
getText(1-5)
🔇 Additional comments (2)
package.json (1)
157-157: Jest transform allowlist update looks good.Allowlisting
cheeriohere is a sensible compatibility fix for test execution with the updated header test surface.client/components/header/header.js (1)
155-197: Good refactor: unified header composition is cleaner.Splitting logos, language switcher, and nav rendering into dedicated helpers improves readability and reduces duplication across desktop/mobile paths.
Update snapshot files to reflect the new unified mobile and desktop HTML structure.
sanjayahari
force-pushed
the
issues/314-enhanchment-title-unify-header
branch
from
927e465 to
c7e47b9
Compare
Refactored the rendering logic for desktop and mobile navigation, logos, and the language switcher to use a single renderNavLinks helper. Added test cases and resolved merge conflicts in yarn.lock. Closes openwisp#314
sanjayahari
force-pushed
the
issues/314-enhanchment-title-unify-header
branch
from
060ff2b to
937ea0f
Compare
Refactored the rendering logic for desktop and mobile navigation, logos, and the language switcher to use a single renderNavLinks helper. Added test cases and resolved merge conflicts in yarn.lock. Closes openwisp#314
Refactored the rendering logic for desktop and mobile navigation, logos, and the language switcher to use a single renderNavLinks helper. Added test cases and resolved merge conflicts in yarn.lock. Closes openwisp#314
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Reference to Existing Issue
Closes #.
Please open a new issue if there isn't an existing issue yet.
Description of Changes
Please describe these changes.
Screenshot
Please include any relevant screenshots.
<img width="1792" height="993" alt="Screenshot 202
6-03-04 004343" src="https://github.com/user-attachments/assets/b7e0bfb1-30da-4794-9660-5c727daa5d6c" />