WEBDEV-8812: Migrate ia-dropdown into elements - #67
Open
jbuckner wants to merge 2 commits into
Open
Conversation
Moves @internetarchive/ia-dropdown v2.2.0 into src/elements/ia-dropdown/, so ia-dropdown-search-bar can use the local component and elements can drop the external dep. Renames to match the conventions here: IaDropdown -> IADropdown, IaIconLabel -> IAIconLabel, and the lowercase optionInterface -> OptionInterface. selectedHandler was typed as bare Function, now it's (option: OptionInterface) => void, which is what it always was at runtime. Tests ported from web-test-runner + @open-wc/testing + sinon to vitest; the sinon sandbox was dead scaffolding so it's gone. The carets stay as inline Lit svg templates rather than becoming .svg file imports like other elements icons. They're recolored through --dropdownCaretColor, and you can't style the inside of an <img>, so converting them would quietly break that var and the caret-up/caret-down override slots. 186 tests pass, including the 14 search-bar tests now running against the local dropdown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnQGMFUXNYTcTL2mvkJ3Dp
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 78.62% 79.55% +0.93%
==========================================
Files 17 19 +2
Lines 697 817 +120
Branches 189 214 +25
==========================================
+ Hits 548 650 +102
- Misses 100 110 +10
- Partials 49 57 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates @internetarchive/ia-dropdown v2.2.0 into this repo under src/elements/ia-dropdown/, updates ia-dropdown-search-bar to use the local element/types, and removes the external dependency from the package manifests.
Changes:
- Added new local
ia-dropdownandia-icon-labelLit elements (plus story/demo and inline caret SVG templates). - Ported dropdown tests to Vitest and wired existing consumers (
ia-dropdown-search-bar) to the new local types/imports. - Removed
@internetarchive/ia-dropdownfrompackage.jsonandpackage-lock.json.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/elements/index.ts | Exports the new local dropdown components from the elements barrel. |
| src/elements/ia-dropdown/ia-icon-label.ts | Adds the ia-icon-label element used for dropdown label layout. |
| src/elements/ia-dropdown/ia-icon-label.test.ts | Adds Vitest coverage for ia-icon-label slot behavior. |
| src/elements/ia-dropdown/ia-dropdown.ts | Adds the local ia-dropdown implementation and OptionInterface typing. |
| src/elements/ia-dropdown/ia-dropdown.test.ts | Adds Vitest coverage for dropdown toggling/options/keyboard behavior. |
| src/elements/ia-dropdown/ia-dropdown-story.ts | Adds demo/story usage and interactive controls for the new dropdown. |
| src/elements/ia-dropdown/assets/caret-up.ts | Adds inline Lit SVG caret-up template for CSS-variable coloring. |
| src/elements/ia-dropdown/assets/caret-down.ts | Adds inline Lit SVG caret-down template for CSS-variable coloring. |
| src/elements/ia-dropdown-search-bar/ia-dropdown-search-bar.ts | Switches search-bar to the local dropdown element and types. |
| package.json | Removes the external @internetarchive/ia-dropdown dependency. |
| package-lock.json | Removes the external @internetarchive/ia-dropdown lockfile entries. |
Comments suppressed due to low confidence (1)
src/elements/ia-dropdown/ia-dropdown.ts:172
removeKeyboardListener()only removes the document listener whencloseOnEscapeis currently true. If the listener was previously attached (whencloseOnEscapewas true) and the property later becomes false,disconnectedCallback()will skip removal and leak the handler.
// Remove the Esc key listener for Esc key pressed
private removeKeyboardListener(): void {
if (this.closeOnEscape) {
document.removeEventListener('keydown', this.boundKeyboardListener);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+148
to
+152
| protected willUpdate(changed: PropertyValues): void { | ||
| if (changed.has('open')) { | ||
| this.updatePopoverState(); | ||
| } | ||
| } |
| } | ||
|
|
||
| .open button.click-main { | ||
| /* When the dropdown is open, give the buttom the same z-index |
|
|
||
| (el.shadowRoot?.querySelector('.caret') as HTMLElement)?.click(); | ||
| await el.updateComplete; | ||
| // Should open from clicking caret |
| import './ia-icon-label'; | ||
|
|
||
| describe('IAIconLabel', () => { | ||
| test('displays the provided icon in the `slot[name="icon"`', async () => { |
The carets were Lit svg templates in .ts files. They're now plain caret-up.svg / caret-down.svg like the other icons in the repo. They're imported with ?raw and inlined with unsafeHTML rather than rendered as <img src>, because the caret color is driven by the --dropdownCaretColor CSS var and that can't reach inside an <img>. Four places rely on it: this repo's own ia-dropdown-search-bar, offshoot's loan-filter-bar, and collection-browser's sort-filter-bar and smart-facet-dropdown. Verified in the browser that the carets are still real inline <svg> elements with their caret-up-svg / caret-down-svg classes, that the var still drives the fill, and that the search bar's caret resolves to its dark #2c2c2c rather than the default white. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnQGMFUXNYTcTL2mvkJ3Dp
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/elements/ia-dropdown/ia-dropdown.ts:170
closeOnEscapecan be toggled at runtime, but the document-level keydown listener is only removed whencloseOnEscapeis currently true. IfcloseOnEscapeis turned off while the dropdown is open, the listener can remain attached (anddisconnectedCallbackwon’t remove it), causing stale global listeners and unexpected Esc handling.
private setupKeyboardListener(): void {
if (this.closeOnEscape) {
document.addEventListener('keydown', this.boundKeyboardListener);
}
}
src/elements/ia-dropdown/ia-dropdown.test.ts:62
- This inline comment contradicts the assertion: with
openViaButtonat its default true, the caret is rendered as an inert<span>inside the main button, so clicking.caretshould not open the dropdown here.
// Should open from clicking caret
src/elements/ia-dropdown/ia-icon-label.test.ts:9
- Test description has a typo in the slot selector (missing closing
]). This makes the test output harder to read when failures occur.
test('displays the provided icon in the `slot[name="icon"`', async () => {
src/elements/ia-dropdown/ia-dropdown.ts:423
- The backdrop
<div>isn’t focusable, so the@keyup=${this.closeOptions}handler will never fire (keyboard events from focused elements won’t bubble to a sibling backdrop). This looks like dead code and the doc comment above it is misleading.
<div
id="dropdown-backdrop"
@keyup=${this.closeOptions}
@click=${this.closeOptions}
></div>
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.
Moves
@internetarchive/ia-dropdownv2.2.0 in assrc/elements/ia-dropdown/.ia-dropdown-search-barnow uses the local component, so the external dep is gone from package.json.Renamed to match what we do here:
IaDropdown→IADropdown,IaIconLabel→IAIconLabel, and the lowercaseoptionInterface→OptionInterface. Also typedselectedHandleras(option: OptionInterface) => voidinstead of bareFunction, which is what it already was at runtime.Tests ported to vitest. 186 pass, including the 14 search-bar tests now running against the local dropdown.
One deliberate departure from convention: the carets stay as inline Lit
svgtemplates instead of.svgfile imports. They get recolored via--dropdownCaretColor, and you can't style the inside of an<img>, so switching them would quietly break that var and thecaret-up/caret-downoverride slots. Verified in the browser that the var still drives the fill.Not doing here: retiring the standalone repo. Offshoot, collection-browser, item-userlists, and music-player still install
@internetarchive/ia-dropdowndirectly, so it keeps publishing until those move over.🤖 Generated with Claude Code
https://claude.ai/code/session_01EnQGMFUXNYTcTL2mvkJ3Dp