feat(atomic-facet): add noFacetSearch attribute to control facet-search #6790
Conversation
louis-bompart
requested review from
alexprudhomme,
Copilot,
jmazzah-coveo and
lavoiesl
louis-bompart
temporarily deployed
to
PR Artifacts
GitHub Actions
Inactive
louis-bompart
temporarily deployed
to
PR Artifacts
GitHub Actions
Inactive
louis-bompart
temporarily deployed
to
PR Artifacts
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR introduces a new no-facet-search attribute to the AtomicFacet component to provide a more intuitive way to disable the facet search feature. The new attribute follows a positive naming pattern (disabling via presence rather than requiring absence) and deprecates the existing with-search attribute. The implementation maintains backward compatibility by combining both attributes in the rendering logic, with noFacetSearch taking precedence.
Key Changes:
- Added
noFacetSearchboolean property to control facet search visibility, replacing the deprecatedwithSearchattribute - Updated rendering logic to respect both attributes during the transition period, with
noFacetSearchtaking precedence - Extended test suite to verify the new property works correctly in isolation and in combination with the legacy
withSearchattribute
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/atomic/src/components/search/atomic-facet/atomic-facet.ts | Added noFacetSearch property, deprecated withSearch, and updated facet search rendering logic to evaluate both properties |
| packages/atomic/src/components/search/atomic-facet/atomic-facet.spec.ts | Added test cases for noFacetSearch property including edge cases with withSearch combinations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Replaces the `with-search` attribute. | ||
| */ | ||
| @property({ | ||
| type: Boolean, |
There was a problem hiding this comment.
The noFacetSearch property is missing the converter: booleanConverter option. All boolean properties in this component and across the Atomic codebase use the booleanConverter to ensure consistent handling of boolean attributes according to HTML standards. The converter also warns users about incorrect usage of boolean attributes (like using value="false"), which will not be supported in Atomic v4.
Add converter: booleanConverter to the @property decorator configuration, similar to other boolean properties like withSearch, isCollapsed, filterFacetCount, and enableExclusion in this same file.
| type: Boolean, | |
| type: Boolean, | |
| converter: booleanConverter, |
| */ | ||
| @property({ | ||
| type: Boolean, | ||
| attribute: 'no-facet-search', |
There was a problem hiding this comment.
The noFacetSearch property is missing the reflect: true option. All similar boolean properties in this component (withSearch, isCollapsed, filterFacetCount, enableExclusion) include reflect: true to synchronize the property value with the HTML attribute. This ensures that when the property changes programmatically, the DOM attribute is updated accordingly, which is important for observability and debugging.
Add reflect: true to the @property decorator configuration for consistency with other boolean properties in this component.
| attribute: 'no-facet-search', | |
| attribute: 'no-facet-search', | |
| reflect: true, |
| it('should render search parts when both noFacetSearch and withSearch are false', async () => { | ||
| const {locators} = await setupElement({ | ||
| noFacetSearch: false, | ||
| withSearch: false, | ||
| }); | ||
| await expect.element(locators.searchWrapper).not.toBeInTheDocument(); | ||
| await expect.element(locators.searchInput).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
The test description states "should render search parts when both noFacetSearch and withSearch are false", but the test assertions verify that search parts are NOT rendered. According to the component logic at line 530 (withSearch: !this.noFacetSearch && this.withSearch), when both are false, the expression evaluates to true && false = false, meaning search parts should NOT be rendered.
The assertions are correct, but the test description is misleading. Change the test description to "should not render search parts when both noFacetSearch and withSearch are false" to accurately reflect what is being tested.
louis-bompart
temporarily deployed
to
Prerelease (CDN)
GitHub Actions
Inactive
louis-bompart
temporarily deployed
to
PR Artifacts
GitHub Actions
Inactive
This pull request introduces a new
no-facet-searchattribute to theAtomicFacetcomponent, allowing consumers to explicitly disable the facet search feature. The existingwith-searchattribute is now deprecated in favor of this new approach. The changes include updates to the component logic, documentation, and test coverage to ensure correct behavior of the new attribute.Facet Search Attribute Improvements:
no-facet-searchboolean property and attribute toAtomicFacet, which disables the facet search feature when set to true. This attribute replaces the previouswith-searchattribute, which is now deprecated. [1] [2]noFacetSearchproperty, ensuring that the search box is only shown whennoFacetSearchis false andwithSearchis true.Testing Enhancements:
noFacetSearchproperty, including combinations with the legacywithSearchproperty, to verify correct rendering of search elements.noFacetSearchproperty. [1] [2]