Skip to content

Restore new combobox + fixes#5670

Merged
edisile merged 7 commits intomainfrom
fix-new-combobox
Apr 17, 2026
Merged

Restore new combobox + fixes#5670
edisile merged 7 commits intomainfrom
fix-new-combobox

Conversation

@edisile
Copy link
Copy Markdown
Contributor

@edisile edisile commented Apr 16, 2026

Done

Reapplied reverted combo box PR (#5617) along with a few fixes. The reverted code has already been reviewed, so you can skip it and review only the new changes by comparing the tip of this branch to the reapply commit: b25b1b5

The reverted version had a problem where navigating to a nested brand store route and refreshing the page would bring you back to /admin/< store id >/snaps rather than the page you were viewing before. Fixes:

  • compare current store id with selected store id in StoreSelector component, call navigate(...) only if they're different
  • improved ComboBox behavior during init (onChange doesn't fire until there are some options to choose from)

As a drive-by I added a primary navigation fix for highlighting the "Models" section when visiting remodel pages

How to QA

Testing

  • This PR has tests (restores previously merged tests)
  • No testing required (explain why):

Security

  • Security considerations for review (list them):
    • Examples:
    • Access control: users can only access their own data
    • Input: user input is validated and sanitised
    • Sensitive data: secret or private data is not exposed in any way
    • ...
  • This PR has no security considerations (explain why):

Issue / Card

Fixes #

Screenshots

UX Approval

  • This PR does not require UX approval
  • This PR does require UX approval (add context):

Copilot AI review requested due to automatic review settings April 16, 2026 08:39
@webteam-app
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reapplies the accessible Downshift-based ComboBox and refactors the publisher-side store switcher to use it, addressing a refresh/navigate regression for nested brand store routes and improving nav highlighting for remodel-related model pages.

Changes:

  • Introduces a new ComboBox component (Downshift) with accompanying SCSS and unit tests.
  • Refactors StoreSelector to use ComboBox and avoids navigating on mount when the selected store matches the URL store id.
  • Adjusts PrimaryNav “Models” highlighting to remain active on nested model/remodel routes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
static/sass/styles.scss Imports and includes the new combobox SCSS mixin in the main stylesheet pipeline.
static/sass/_snapcraft_p-combobox.scss Adds new .p-combobox styling used by the Downshift-based component.
static/sass/_snapcraft_l-application.scss Removes the old .store-selector styling that is no longer used after the refactor.
static/js/publisher/components/StoreSelector/StoreSelector.tsx Replaces custom store selector UI with the new ComboBox and guards navigation on mount.
static/js/publisher/components/StoreSelector/tests/StoreSelector.test.tsx Adds tests to ensure StoreSelector renders correctly and only navigates on actual changes.
static/js/publisher/components/PrimaryNav/PrimaryNav.tsx Updates “Models” nav selection logic to cover nested remodel/model routes.
static/js/publisher/components/ComboBox/ComboBox.tsx Adds the new ComboBox component and state/behavior patches around Downshift.
static/js/publisher/components/ComboBox/tests/ComboBox.test.tsx Adds unit tests for ComboBox interactions, filtering, and keyboard behavior.

Comment thread static/js/publisher/components/ComboBox/ComboBox.tsx Outdated
Comment thread static/js/publisher/components/ComboBox/__tests__/ComboBox.test.tsx Outdated
@edisile edisile changed the title Fix new combobox Restore new combobox + fixes Apr 16, 2026
@ilayda-cp ilayda-cp self-requested a review April 16, 2026 10:29
"aria-current":
location.pathname === `/admin/${storeId}/models`,
"aria-current": location.pathname.includes(
`/admin/${storeId}/models`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

changing to "includes" makes it too loose. It might work now but in the future if we add something like "/admin/${storeId}/models-archive" it would cause a bug.

@edisile edisile merged commit 884e0de into main Apr 17, 2026
14 checks passed
@edisile edisile deleted the fix-new-combobox branch April 17, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants