[change] extracted Header logic into reusable methods to remove HTML #314#1032
[change] extracted Header logic into reusable methods to remove HTML #314#1032prathmeshkulkarni-coder wants to merge 4 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe header component in client/components/header/header.js was refactored to remove duplicated inline rendering by introducing three methods on the Header class: renderLogos(isMobile = false), renderLanguageButtons(isMobile = false), and renderNavLinks(isMobile = false). The main render now calls these helpers for both desktop and mobile contexts (including the mobile overlay and hamburger trigger), preserving existing state-driven behavior and sticky message logic. Other unrelated small changes appear across the diff (babel.config.js, client/index.js, client/components/status/status.js, package.json). Lines changed: +109 / -175. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
client/components/header/header.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (6)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/reducers/language.js (1)
language(3-10)client/actions/set-language.js (1)
setLanguage(3-6)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] 110-137: Expected blank line between class members.
(lines-between-class-members)
[error] 138-184: Expected blank line between class members.
(lines-between-class-members)
[error] 140-140: 'language' is assigned a value but never used.
(no-unused-vars)
[error] 157-157: A control must be associated with a text label.
(jsx-a11y/control-has-associated-label)
[error] 157-157: Visible, non-interactive elements with click handlers must have at least one keyboard listener.
(jsx-a11y/click-events-have-key-events)
[error] 157-157: Elements with the 'button' interactive role must be focusable.
(jsx-a11y/interactive-supports-focus)
⏰ 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
🤖 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 110: Add missing blank lines between class members to satisfy the ESLint
lines-between-class-members rule: insert a blank line above the renderNavLinks
method declaration (renderNavLinks = (isMobile = false) => {) and another blank
line above the render method (render() {) so each class member is separated by a
blank line; update the class containing renderNavLinks and render accordingly.
- Around line 64-93: The method bodies for renderLogos, renderLanguageButtons,
renderNavLinks and the render method are mis-indented relative to the class; fix
by adjusting their block indentation to match the class nesting (indent the
contents of each method one level in from the method declaration, e.g.,
consistent 2/4-space style used across the file), ensuring JSX return blocks and
inner consts (e.g., const { header, orgSlug } in renderLogos) are indented
consistently with the other class methods so auto-formatters and reviewers see a
uniform indentation level.
- Around line 64-93: In renderLogos, the second logo is only checked for
truthiness (secondLogo) but its url is accessed directly causing
getAssetPath(orgSlug, secondLogo.url) to receive undefined; update the condition
to require secondLogo?.url (or check secondLogo && secondLogo.url) and use
optional chaining when reading secondLogo.url and secondLogo.alternate_text
(e.g., secondLogo?.url, secondLogo?.alternate_text) so getAssetPath and the img
alt only run when a valid URL exists.
- Around line 138-141: Remove the unused destructured variable by deleting
"const { language } = this.props;" from the render() method; keep "const { menu
} = this.state;" and ensure any helper calls continue to read language from
this.props (or, if preferred, replace helper calls to use the locally
destructured language consistently) so there are no no-unused-vars ESLint
violations in the Header component's render method.
- Around line 110-137: In renderNavLinks, the external/fallback anchor is using
the raw link.url which leaves the {orgSlug} placeholder unresolved; change the
anchor's href to use resolvedUrl (the value computed from
link.url.replace("{orgSlug}", orgSlug)) instead of link.url so all links with
{orgSlug} are substituted correctly; update the <a> rendering branch that
currently uses key={link.url} to use resolvedUrl for href (and optionally as the
key) while keeping the same className, target and rel attributes.
- Around line 155-162: The header hamburger div with className
"header-hamburger" and role="button" is missing keyboard accessibility
attributes; update that element to include tabIndex={0},
onKeyUp={this.handleKeyUp}, and a descriptive aria-label (e.g., "Toggle menu")
so it becomes focusable, operable via keyboard, and announced to assistive tech;
keep the existing onClick={this.handleHamburger} and the inner divs using the
menu state as-is.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
client/components/header/header.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (5)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/actions/set-language.js (1)
setLanguage(3-6)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)
⏰ 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 comments (3)
client/components/header/header.js (3)
95-109:renderLanguageButtonslooks good.Past ESLint blank-line issue is resolved, indentation is consistent, and the active class logic is correct.
111-138:resolvedUrlcorrectly used in external<a>— past bug resolved.The
{orgSlug}substitution is now applied consistently for all link variants.
140-165: Accessibility and unused-variable issues from prior review are resolved.The hamburger element now carries
tabIndex={0},aria-label="Toggle menu", andonKeyUp={this.handleKeyUp}, and the staleconst { language } = this.propsdestructuring has been removed fromrender().
🤖 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 149-151: Remove the trailing whitespace on the blank line after
the div in the header component so linters no longer flag it; locate the
fragment around the JSX in the Header component (near the <div
className="header-left">{this.renderLogos()}</div> and the subsequent "Desktop
Languages" comment) and delete the stray spaces on the empty line or collapse
the blank line so it contains no trailing spaces.
- Around line 64-93: renderLogos currently accepts an isMobile flag but is only
called without it, so deviceClass always becomes "desktop"; to fix, keep the
isMobile parameter and update the mobile menu overlay code that renders the
header (the mobile overlay in render) to call this.renderLogos(true) so the
mobile CSS class (header-mobile-logo-image) is applied; locate renderLogos and
the mobile overlay JSX that currently renders logos (inside the header render
method) and replace or add the call to this.renderLogos(true).
- Line 115: internalLinks is being reallocated every render because it's defined
inside header.js and used by renderNavLinks which is called twice (desktop and
mobile); move the computation out of renderNavLinks by defining internalLinks
once and reusing it — either compute it once inside render() and pass it into
both calls to renderNavLinks, or lift it to a module/class-level constant (e.g.,
a static property on the Header component) so renderNavLinks and the
mobile/desktop invocations use the same precomputed array.
---
Duplicate comments:
In `@client/components/header/header.js`:
- Around line 64-93: The method body of renderLogos is misindented relative to
its declaration; adjust the indentation of the lines inside renderLogos
(starting with the const { header, orgSlug } = this.props; and subsequent lines)
so that the method declaration renderLogos = (isMobile = false) => { is at one
level and all inner statements (const, return, JSX) are indented one additional
level inside the function body, keeping existing variable names (header,
orgSlug, logo, secondLogo, deviceClass) and JSX structure unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
client/components/header/header.js
🧰 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)
⏰ 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 comments (3)
client/components/header/header.js (3)
64-93:isMobileparameter is now properly exercised.The mobile overlay on line 179 calls
this.renderLogos(true), which was the intended fix from the prior review. Bothheader-desktop-logo-imageandheader-mobile-logo-imageclass variants are now reachable.
95-109: LGTM — clean and correct implementation.Language button rendering is correctly parameterized by device type and keyed by
lang.slug.
140-189: LGTM — all previously flagged issues addressed.
languageis no longer unnecessarily destructured inrender().- The hamburger
<div>now hastabIndex={0},aria-label, andonKeyUp={this.handleKeyUp}.renderLogos(true)is correctly placed in the mobile overlay (line 179).- Desktop and mobile paths consistently delegate to the three helper methods.
🤖 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 125: The conditional in the header rendering uses
internalLinks.indexOf(link.url) which compares unresolved template URLs (e.g.,
"/{orgSlug}/login") against resolved paths, so change the check to use the
resolved URL variable (resolvedUrl) instead of link.url in the if condition
inside the component (the same resolvedUrl used later at line where resolvedUrl
is compared on line 133); update the conditional in the function that
renders/filters links (e.g., shouldLinkBeShown/where the if (isInternal && ...)
lives) and adjust or add a unit test to pass a config containing
"/{orgSlug}/login" so the resolvedUrl branch is exercised.
---
Duplicate comments:
In `@client/components/header/header.js`:
- Line 115: internalLinks is being reallocated each time renderNavLinks runs;
move its construction out of renderNavLinks so it's created once per render (or
once per component) — e.g., compute const internalLinks = ["/login",
"/registration"].map(p => `/${orgSlug}${p}`) inside render() and pass
internalLinks into renderNavLinks, or store it as a class/instance field
computed when orgSlug changes; update renderNavLinks signature/uses accordingly
to reference the precomputed internalLinks (symbols: internalLinks,
renderNavLinks, orgSlug).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
client/components/header/header.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (6)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/reducers/language.js (1)
language(3-10)client/actions/set-language.js (1)
setLanguage(3-6)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)
⏰ 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 comments (3)
client/components/header/header.js (3)
64-93: LGTM — clean extraction of logo rendering.The
renderLogoshelper properly handles both device contexts, uses optional chaining for safety, and is called correctly for desktop (line 161) and mobile (line 191).
95-109: LGTM — language button rendering is clean.Correct use of
lang.slugas key, proper device-class parameterization, and concise mapping logic.
152-181: LGTM — render method is clean and well-structured.Accessibility attributes on the hamburger (
tabIndex,aria-label,onKeyUp) are properly in place. Desktop/mobile sections are clearly delineated with comments. Unusedlanguagedestructuring has been removed.
🤖 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 183-196: The header currently renders logos, nav links, and
language buttons twice via renderLogos, renderNavLinks, and
renderLanguageButtons (once for desktop and once for mobile), producing
duplicate DOM; change header.js to render each helper only once (remove the
duplicate desktop/mobile helper calls and their wrappers like header-row-2
header-desktop-only and header-mobile-menu header-mobile-only) and rely on
responsive CSS classes/media queries to show/hide or reflow the single DOM tree
for different viewports, ensuring layout differences are handled purely in CSS
while keeping the unique helper calls (renderLogos, renderNavLinks,
renderLanguageButtons) in a single render pass.
- Around line 120-149: The map callback that renders header links uses
inconsistent keys (key={index} for internal branch vs key={resolvedUrl} for
external), which can break React reconciliation; update the internal-link branch
in the header render (inside the links?.map(...) callback that calls
shouldLinkBeShown, isInternalLink, resolvedUrl, and getText) to use the same
stable key as the fallback branch (use key={resolvedUrl}) so every rendered
<Link> and <a> in that map share a consistent, unique key strategy.
---
Duplicate comments:
In `@client/components/header/header.js`:
- Around line 111-118: The internalLinks array is being recomputed inside
renderNavLinks each time it's called; move its creation out of renderNavLinks so
it’s computed only once per render. Either hoist a module-level constant (if
orgSlug is static) or compute internalLinks in render() using this.props.orgSlug
and pass it into renderNavLinks (e.g., renderNavLinks(internalLinks, isMobile)),
then update the renderNavLinks signature and call sites to use the precomputed
internalLinks; reference renderNavLinks, internalLinks, render(), and orgSlug
when making the change.
| {/* Row 2: Desktop Navigation */} | ||
| <div className="header-row-2 header-desktop-only"> | ||
| <div className="header-row-2-inner">{this.renderNavLinks()}</div> | ||
| </div> | ||
| {/* Mobile Menu Overlay */} | ||
| <div | ||
| className={`${menu ? "display-flex" : "display-none"} header-mobile-menu`} | ||
| className={`${menu ? "display-flex" : "display-none"} header-mobile-menu header-mobile-only`} | ||
| > | ||
| {links && | ||
| links.map((link, index) => { | ||
| if (shouldLinkBeShown(link, isAuthenticated, userData)) { | ||
| if (isInternalLink(link.url)) { | ||
| return ( | ||
| <Link | ||
| className={`header-link mobile-link | ||
| header-link-${index + 1} ${ | ||
| pathname === link.url.replace("{orgSlug}", orgSlug) | ||
| ? "active" | ||
| : "" | ||
| } button`} | ||
| to={link.url.replace("{orgSlug}", orgSlug)} | ||
| key={index} | ||
| > | ||
| {getText(link.text, language)} | ||
| </Link> | ||
| ); | ||
| } | ||
| return ( | ||
| <a | ||
| href={link.url} | ||
| className={`header-link mobile-link | ||
| header-link-${index + 1} button`} | ||
| target="_blank" | ||
| rel="noreferrer noopener" | ||
| key={link.url} | ||
| > | ||
| {getText(link.text, language)} | ||
| </a> | ||
| ); | ||
| } | ||
| return null; | ||
| })} | ||
| {this.renderLogos(true)} | ||
| {this.renderNavLinks(true)} | ||
| <div className="mobile-languages-row"> | ||
| {languages.map((lang) => ( | ||
| <button | ||
| type="button" | ||
| className={`${ | ||
| language === lang.slug ? "active " : "" | ||
| }header-language-btn header-mobile-language-btn header-language-btn-${ | ||
| lang.slug | ||
| }`} | ||
| key={lang.slug} | ||
| onClick={() => setLanguage(lang.slug)} | ||
| > | ||
| {lang.text} | ||
| </button> | ||
| ))} | ||
| {this.renderLanguageButtons(true)} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Desktop and mobile sections still render duplicate DOM elements.
The linked issue #314 objective is to "consolidate header rendering so a single implementation, styled with CSS, can serve all viewports." The current refactor calls each helper twice (desktop + mobile), producing two full copies of logos, nav links, and language buttons in the DOM. While the extraction into helpers eliminates code duplication, the DOM duplication remains — the CSS display-none/display-flex toggle just hides one copy.
Consider whether a single render pass with responsive CSS (e.g., media queries or container queries) could replace the two-pass approach, which would fully satisfy the linked issue's intent and halve the rendered DOM nodes.
🤖 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 183 - 196, The header
currently renders logos, nav links, and language buttons twice via renderLogos,
renderNavLinks, and renderLanguageButtons (once for desktop and once for
mobile), producing duplicate DOM; change header.js to render each helper only
once (remove the duplicate desktop/mobile helper calls and their wrappers like
header-row-2 header-desktop-only and header-mobile-menu header-mobile-only) and
rely on responsive CSS classes/media queries to show/hide or reflow the single
DOM tree for different viewports, ensuring layout differences are handled purely
in CSS while keeping the unique helper calls (renderLogos, renderNavLinks,
renderLanguageButtons) in a single render pass.
prathmeshkulkarni-coder
force-pushed
the
refactor-header-dry
branch
from
b86cc3d to
948c6d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
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/header.js (1)
1-1: 🧹 Nitpick | 🔵 TrivialStale
eslint-disable react/no-array-index-key—indexis no longer used as a JSX key.The file-wide disable was needed when
key={index}was used in the links map. After the fix that changed both branches tokey={resolvedUrl}(lines 133 and 144),indexis now only referenced inside theclassNametemplate literal (line 126), which thereact/no-array-index-keyrule does not flag. The suppress comment can be safely removed.♻️ Proposed fix
-/* eslint-disable react/no-array-index-key */ import "./index.css";🤖 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, Remove the now-stale eslint-disable comment that suppresses react/no-array-index-key at the top of header.js; the component (Header) no longer uses index as a JSX key—keys are set to resolvedUrl in the map (key={resolvedUrl}) and index is only used inside the className template literal—so delete the file-level "eslint-disable react/no-array-index-key" comment and run lint to confirm no rule violations remain.
📜 Review details
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 (5)
babel.config.jsclient/components/header/header.jsclient/components/status/status.jsclient/index.jspackage.json
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/header/header.js (7)
client/utils/get-asset-path.js (1)
getAssetPath(1-1)client/utils/load-translation.js (1)
lang(39-39)client/reducers/language.js (1)
language(3-10)client/actions/set-language.js (1)
setLanguage(3-6)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 (1)
client/components/header/header.js (1)
64-93:renderLogos,renderLanguageButtons, andrenderNavLinkshelpers — LGTM.Past issues are addressed:
renderLogos(true)is correctly called in the mobile overlay (line 191),resolvedUrlis used consistently in both link branches,key={resolvedUrl}is uniform,internalLinksuses resolved paths, and the unusedlanguagedestructuring inrender()is removed. The hamburger also correctly hastabIndex,aria-label, andonKeyUp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@babel.config.js`:
- Line 17: Remove the unsupported option object from the plugin entry for
"@babel/plugin-syntax-dynamic-import": locate the array element
["@babel/plugin-syntax-dynamic-import",{loose: true}] and change it to just
"@babel/plugin-syntax-dynamic-import" (i.e., remove the { loose: true } object)
because this is a syntax-only plugin and the loose option is ignored.
- Line 14: The plugin entry for "@babel/plugin-transform-runtime" includes an
unsupported option "{loose: true}"; remove the "loose" property from that plugin
config (or replace it with a valid option such as "useESModules", "corejs",
"helpers", "regenerator", "absoluteRuntime", "moduleName" or "version" depending
on the intended behavior). Locate the plugin entry string
"@babel/plugin-transform-runtime" in the babel.config.js and either delete the
loose key or set a correct supported option (e.g., useESModules: true) to
achieve the intended effect.
In `@client/components/status/status.js`:
- Line 1231: The inline debug style on the status container div
(className="container content flex-wrapper", id="status") must be removed
because it forces a blue background and white border in production; delete the
style prop entirely from that div and, if the appearance change is intentional,
move the styles into the appropriate CSS/SCSS rule for the `#status` selector or
an existing class so styling is controlled via stylesheets rather than an inline
development artifact.
- Line 1222: Remove the development debug statement console.log("GEMINI: Status
component is rendering!") from the Status component so it no longer logs on
every render; locate the console.log in the Status component (status.js) and
either delete it or gate it behind a debug flag (e.g., process.env.NODE_ENV !==
'production') and ensure no other console.* debug statements remain in the
component.
In `@client/index.js`:
- Line 1: Remove the development debug statement console.log("hello") from the
top of the module (it appears before any imports and violates the import/first
ESLint rule); simply delete that console.log line so there is no top-level debug
output left in the client/index.js module.
In `@package.json`:
- Line 110: The package.json scripts are inconsistent: the "client" script
doesn't set NODE_OPTIONS=--openssl-legacy-provider while "build" and "build-dev"
still do; decide whether the legacy OpenSSL flag is required and make scripts
consistent—either add NODE_OPTIONS=--openssl-legacy-provider to the "client"
script (the same way "build" and "build-dev" do) if compatibility is needed, or
remove the flag from "build" and "build-dev" if it is no longer necessary;
update the "client", "build", and "build-dev" script entries accordingly so all
three use the same NODE_OPTIONS behavior.
- Line 73: package.json currently lists the dependency "cross-env" but no
scripts use it; either remove the unused dependency or update the scripts that
rely on Unix-style env assignment (the "build" and "build-dev" npm scripts) to
use cross-env. To fix, either delete the "cross-env" entry from
dependencies/devDependencies, or modify the "build" and "build-dev" script
values to wrap NODE_OPTIONS with cross-env (e.g., use cross-env NODE_OPTIONS=...
before the existing command) so the package is actually used and cross-platform;
update package.json accordingly and run npm install to sync lockfile.
---
Outside diff comments:
In `@client/components/header/header.js`:
- Line 1: Remove the now-stale eslint-disable comment that suppresses
react/no-array-index-key at the top of header.js; the component (Header) no
longer uses index as a JSX key—keys are set to resolvedUrl in the map
(key={resolvedUrl}) and index is only used inside the className template
literal—so delete the file-level "eslint-disable react/no-array-index-key"
comment and run lint to confirm no rule violations remain.
---
Duplicate comments:
In `@client/components/header/header.js`:
- Around line 184-196: The header currently renders duplicate DOM for desktop
and mobile by calling renderNavLinks() and renderLanguageButtons() twice;
instead render each helper only once and conditionally show/move it based on
viewport/menu state—e.g., compute a single navNode =
this.renderNavLinks(isMobile) and languageNode =
this.renderLanguageButtons(isMobile) (or extract to a standalone component) and
render those nodes only where needed (use conditional rendering tied to menu or
viewport detection, or use a React Portal to move the single instance into the
mobile overlay when menu is open). Update references to renderNavLinks,
renderLanguageButtons, and renderLogos so they produce a single DOM instance
rather than duplicated outputs.
efa63d1 to
64c49cd
Compare
prathmeshkulkarni-coder
changed the title
prathmeshkulkarni-coder
force-pushed
the
refactor-header-dry
branch
5 times, most recently
from
9475c50 to
8595cdc
Compare
|
you have to use a valid prefix in the commit message, the list of valid prefixes is: Inside the square brackets, replace |
i was mentioning your comment xd |
prathmeshkulkarni-coder
force-pushed
the
refactor-header-dry
branch
4 times, most recently
from
c237923 to
cfbd2a1
Compare
prathmeshkulkarni-coder
changed the title
|
please read the contributing docs. Its a good practice to read it before contributing |
Extracted JSX into helper methods to reduce duplication and improve maintainability Fixes openwisp#314
prathmeshkulkarni-coder
force-pushed
the
refactor-header-dry
branch
from
cfbd2a1 to
f9212b2
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This is one the issues that are part of one of our GSoC 26 ideas.
If you want me to take you seriously you should follow our guidelines: this issue is not listed in the boards that we tell contributors to look for picking up issues.
So you started working on something most likely due to GSoC, but seems you're not taking the time to learn about our process first and want to jump straight to coding.
This is not what we are looking for. I won't be reviewing this for now.
Please read our guidelines, start from: https://github.com/orgs/openwisp/discussions/1181.
5 participants
Checklist
Reference to Existing Issue
Closes #314.
Please open a new issue if there isn't an existing issue yet.
Description of Changes
As part of the modernization effort outlined in #314, I have refactored the Header.js component to remove structural duplication.
Improvements:
DRY Implementation: Extracted rendering logic into renderLogos, renderNavLinks, and renderLanguageButtons.
Reduced JSX Bloat: Eliminated redundant HTML blocks that were previously mirrored for desktop and mobile views.
Maintainability: Standardized how internal/external links are processed across all viewports.
This refactor simplifies the component's structure, making it a cleaner candidate for the React 19 migration.
Screenshot
Mobile View
Desktop View