Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the atomic-sort-dropdown component did not properly respect tabs-included attributes after Atomic version 3.39. The fix ensures that when the active tab changes, the sort expression is reset if it's no longer valid for the current tab.
Changes:
- Modified the tab manager state binding to use a callback method for state changes
- Refactored the sort validation logic into a separate method for improved code organization
- Added a test case to verify the tab state change callback behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/atomic/src/components/search/atomic-sort-dropdown/atomic-sort-dropdown.ts |
Introduced onTabManagerStateChange callback to react to tab changes and reset invalid sort expressions; extracted validation logic into isSortValid method |
packages/atomic/src/components/search/atomic-sort-dropdown/atomic-sort-dropdown.spec.ts |
Added test case verifying sort reset behavior when tab state changes via the new callback mechanism |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…mic-sort-dropdown.spec.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@y-lakhdar I've opened a new pull request, #6995, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 303687a02e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Issue
After the Stencil→Lit migration (#6658), atomic-sort-dropdown stopped resetting the sort criteria when switching to a tab where the current sort option isn't allowed (via tabs-included/tabs-excluded).
Root Cause
The Stencil version validated sort criteria in
componentShouldUpdate(), which ran synchronously on every state change including tab switches. The Lit migration moved this toupdated(), which doesn't reliably trigger when tabManagerState changes.Context
https://discuss.coveo.com/t/support-00133057-atomic-sort-expression-not-respecting-tabs-included-after-atomic-version-3-39/14095
https://coveord.atlassian.net/browse/KIT-5419