Skip to content

Refactor HeaderNavigation button styles - #5421

Open
MaddipatlaChetan24 wants to merge 1 commit into
uber:mainfrom
MaddipatlaChetan24:patch-9
Open

MaddipatlaChetan24 wants to merge 1 commit into
uber:mainfrom
MaddipatlaChetan24:patch-9

Conversation

@MaddipatlaChetan24

Copy link
Copy Markdown
Contributor

Refactored styles for navigation buttons to use constants for better performance and readability.

Fixes #1, Fixes #2

Description

Two changes in header-navigation.jsx:

  1. Removed an unused import (SlackLogo from ./slack-logo) — confirmed
    via a full-file scan that it's never referenced anywhere in the
    component's render output.

  2. Hoisted four overrides.BaseButton.style objects to module-level
    constants: NAV_LINK_BUTTON_OVERRIDE_STYLE (shared by the Blog and
    Components buttons, whose override shapes are byte-for-byte
    identical — verified with a deep-equality check), plus separate
    constants for the GitHub, Direction Toggle, and Theme Toggle
    buttons' overrides. None of these four reference theme, props, or
    any other component-local variable — they're pure functions of
    mq(...) calls with constant arguments — so they were previously
    being re-created as new object literals on every single render of
    HeaderNavigation for no benefit.

Left the Nav Toggle button's override inline, since it references
theme.mediaQuery.medium (a component-scoped value) and can't be
safely hoisted without knowing whether that theme property is
guaranteed constant across every theme variant this app uses.

Also removed a stray, empty // comment line between the copyright
header and the imports — vestigial, no functional effect either way.

No visual or behavioral changes — same resolved styles, same rendered
output.

Scope

Patch: Cleanup

Refactored styles for navigation buttons to use constants for better performance and readability.
@MaddipatlaChetan24

Copy link
Copy Markdown
Contributor Author

please check this !!

@dyesin dyesin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Before we review it, please note that "check this" isn't a submission we can accept. It hands the verification work to maintainers, and our review time is limited.

We welcome AI-assisted contributions, but you are the author and must stand behind every line. Before requesting review, please:

Run it. Set up the dev environment, run the full test suite, and confirm the change works as intended.
Understand it. Be ready to explain why each change was made and what could break. "The model suggested it" isn't an answer.
Justify it. Describe the actual problem this solves and how you confirmed it's fixed. For performance changes, include evidence.
Test it. Add or update tests following the patterns already used in the repo.
Keep it focused. One concern per PR, with no unrelated edits.

Once you can confirm all of the above in the PR description, we're happy to take a look. Until then, we'll mark this as a draft.

@MaddipatlaChetan24

Copy link
Copy Markdown
Contributor Author

I have tested the system and successfully run the build without any errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants