fix: unify chip-remove event — both modes emit ol-filter-change#40
Merged
mekarpeles merged 2 commits intomainfrom Apr 27, 2026
Merged
fix: unify chip-remove event — both modes emit ol-filter-change#40mekarpeles merged 2 commits intomainfrom
mekarpeles merged 2 commits intomainfrom
Conversation
closes #34 Previously _handleChipRemove dispatched ol-chip-remove in embedded mode and ol-filter-change (via _emitFilter) in droppable mode. ol-search-page maintained a separate _onChipRemove handler wired to ol-chip-remove in addition to _onFilterChange. Now the embedded branch computes the new filter value from _localFilters (same logic as the droppable branch) and dispatches ol-filter-change with { filter, value }. ol-search-page's _onFilterChange already handles every filter field so _onChipRemove and its event listener are removed. Four static-analysis tests in ol-search-bar.events.test.js verify the contract: no ol-chip-remove in either source file, _handleChipRemove dispatches ol-filter-change, _onFilterChange covers all seven fields.
Contributor
There was a problem hiding this comment.
Pull request overview
Unifies chip removal behavior so ol-search-bar emits a single ol-filter-change event (with { filter, value }) in both droppable and embedded modes, removing the legacy ol-chip-remove event path.
Changes:
- Updated
ol-search-barembedded-mode chip removal to dispatchol-filter-changeinstead ofol-chip-remove. - Removed
ol-chip-removelistener and handler fromol-search-pageto eliminate duplicated state-update logic. - Added static-analysis tests and updated docs to reflect the unified event API.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/ol-search-page.js | Removes ol-chip-remove wiring/handler; relies solely on ol-filter-change. |
| frontend/src/components/ol-search-bar.js | Updates embedded chip × behavior to dispatch ol-filter-change with { filter, value }. |
| frontend/src/components/ol-search-bar.events.test.js | Adds static-analysis tests asserting removal of ol-chip-remove and coverage of filter fields. |
| frontend/src/components/ol-catalog.js | Updates component catalog documentation to reference ol-filter-change. |
| frontend/src/components/README.md | Updates docs to remove ol-chip-remove and describe unified ol-filter-change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- _handleChipRemove embedded path now uses this.filters (prop) as source of truth for multi-value array lookups; falls back to _localFilters only if filters prop was not provided — prevents clearing an entire genre/author/subject array when the host passes filters correctly - JSDoc updated: embedded mode now documents that both .chips and .filters are required for correct multi-value chip removal - catalog.js embedded example now passes .filters alongside .chips - README updated to note that .filters is required in embedded mode
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34
What changed
ol-search-bar.js—_handleChipRemoveembedded branch now computes the new filter value from_localFilters(same logic as the droppable branch) and dispatchesol-filter-changewith{ filter, value }instead ofol-chip-removewith{ type, value }.ol-search-page.js—_onChipRemovehandler and itsol-chip-removeevent listener removed._onFilterChangealready handled all seven filter fields correctly, so no logic was lost — just the duplicate handler.ol-catalog.js+README.md— documentation updated to reflect the unified event.Why
A host of
ol-search-barpreviously needed two listeners (ol-chip-removeandol-filter-change) with two different payload shapes to handle the same user action (clicking × on a chip) depending on which mode the component was in. Now any host listens for exactly one event.Verified by
4 new static-analysis tests in
ol-search-bar.events.test.js:ol-search-barsource contains nool-chip-removeol-search-pagesource contains nool-chip-remove_handleChipRemovedispatchesol-filter-change_onFilterChangecovers all seven filter fields171 tests pass, lint clean.