Add item card display variants#27
Conversation
|
Correction to the PR body formatting: this adds official |
raimannma
left a comment
raimannma
left a comment
There was a problem hiding this comment.
Automated /code-review (high effort, 7 finder angles → verified). 9 findings below as inline comments — 2 user-visible state bugs on the new variants, 1 clean reuse fix, plus behavior/visual regressions and cleanup. The fetchItems-duplication and wrapper-regeneration candidates were refuted (caching is present; React/Vue wrappers consume the widened type transitively from dist).
| } | ||
|
|
||
| private renderInlineItem(item: Item | undefined, isClickMode: boolean, parts: { image: boolean; name: boolean }) { | ||
| if (this._loading || !item) return <span class="item-inline-trigger loading"></span>; |
There was a problem hiding this comment.
Loading skeleton is invisible for the inline / image-name variants. When variant is inline/inline-text/inline-image/image-name and the item is fetched via class-name/item-id, _loading is true and this returns an empty <span>/<div class="... loading">. But the host and .item-inline-trigger/.item-image-name are width:auto;height:auto with no content and no min-size, so the #1a1a2e shimmer paints a 0×0 box — no loading feedback at all. (The fixed-square image/icon variants are fine since the host pins 48px.) Consider a min-width/min-height or a placeholder glyph for the auto-sized loading state.
|
|
||
| private renderImageOnly(item: Item | undefined, isClickMode: boolean) { | ||
| if (this._loading || !item) return <div class="item-image-only loading"></div>; | ||
| if (this._error) return <div class="item-image-only error" title={this._error}></div>; |
There was a problem hiding this comment.
Error state is effectively invisible for variant="image" and variant="inline-image". On fetch failure renderImageOnly returns a blank square (no child) and renderInlineItem with parts.name=false renders {null} — an empty 0-width span. There is no .error CSS rule, so the only signal is the hover-only title attr, which is inaccessible on touch and invisible otherwise. (image-name/inline/inline-text do show the error text.) Consider an error glyph/fallback for the image-only paths.
| } | ||
| } | ||
|
|
||
| private getImageSrc(item: Item): string | undefined { |
There was a problem hiding this comment.
Nice extraction — but the identical fallback chain is still inlined in resolveComponentItems (line ~213) and resolveParentItems (line ~240): x.shop_image_webp || x.shop_image || x.image_webp || x.image || undefined. Both comp and other are typed Item, so this.getImageSrc(comp) / this.getImageSrc(other) are drop-in replacements. Otherwise the precedence now lives in 3 places and a future change updates the cards but silently leaves the tooltip thumbnails on the old chain.
| ]; | ||
| } | ||
|
|
||
| private renderVariant(item: Item | undefined, isClickMode: boolean) { |
There was a problem hiding this comment.
Altitude: the new lightweight variants still pay the full card cost. fetchItemData() always runs resolveComponentItems + resolveParentItems (the latter does an O(n) scan over the entire item catalog) regardless of variant, and render() always mounts dl-item-tooltip unless trigger='none'. A page with many inline-image chips therefore does N catalog scans + N hidden tooltip mounts for purely decorative glyphs. (fetchItems is cached so network is bounded, but the per-card scan + DOM mount are not.) Worth gating the parent/component resolution and tooltip mount on whether they're actually needed.
| height: 100%; | ||
| position: relative; | ||
| border-radius: 4px; | ||
| border-radius: 0; |
There was a problem hiding this comment.
.icon-box border-radius changed 4px → 0, which silently squares the corners of the existing variant="icon" usage. If this is intentional (to match the new square image variant), worth calling out in the PR description / changelog; otherwise it's an unannounced visual regression for current consumers.
| } | ||
|
|
||
| :host(.custom-trigger) { | ||
| :host(.custom-trigger), |
There was a problem hiding this comment.
Altitude: variant classification is enumerated by name in 2–3 separate CSS selector lists (this :host sizing block, the .trigger block below, and the icon/image group) plus the renderVariant switch. Adding a variant means editing the type, the switch, and every CSS list in lockstep — miss one and the variant sizes wrong. The existing custom-trigger marker class (toggled once in detectCustomTrigger) is the pattern that would collapse these into a single rule (e.g. an auto-size host class).
| return item?.item_slot_type ?? 'neutral'; | ||
| } | ||
|
|
||
| private renderImageOnly(item: Item | undefined, isClickMode: boolean) { |
There was a problem hiding this comment.
Simplification: renderImageOnly / renderImageName / renderInlineItem (and renderIcon) repeat near-identical if (_loading||!item) return loading; if (_error) return error; + getImageSrc + clickable/slot-class boilerplate, differing only in tag and which parts render. A small shared helper for the guard + class object would mean a change to the loading/error contract is made once instead of 4×.
| width: 100%; | ||
| } | ||
|
|
||
| .item-image-only.clickable, |
There was a problem hiding this comment.
Duplication (minor): the clickable/loading/cursor trio is now repeated in three blocks (.mod-box, .icon-box, and this new .item-image-only/.item-image-name/.item-inline-trigger group) — a shared base selector would keep the shimmer timing/cursor behavior from drifting. Also the --item-accent-color default + color declaration is duplicated verbatim in both the .item-image-name and .item-inline-trigger base rules. (Note: the per-slot weapon/vitality/spirit accent rules are already comma-grouped across the two selectors, so those are fine.)
2 participants
Summary
dl-item-cardvariants for image-only, image+name, inline image+text, inline text, and inline image triggerscardbehavior while making the card image area explicitly square and aligning name typography with the shop card stylingItemCardVariantthrough core types so generated React/Vue wrappers get typed variantstooltip-triggerprop; variants still open tooltips unless disabled withtooltip-trigger="none"variant="icon"now defaults to square corners to match the square source item art; consumers can override with--dl-item-icon-border-radiusVerification
npm run build(Stencil core + React wrapper + Vue wrapper)