feat!: attribute reduction — client-side open/close + template migration#292
feat!: attribute reduction — client-side open/close + template migration#292
Conversation
Move dropdown, menu, popover, drawer, datepicker, timepicker, and autocomplete open/close from server-managed state to client-side CSS class toggling. This eliminates server round-trips for UI-only concerns like dropdown visibility, click-away closing, and toggle toggling. Pattern: - Root element gets lvt-el:removeClass:on:click-away="open" - Trigger button uses onclick JS to toggle "open" class - Panel is always rendered, hidden via CSS when root lacks "open" - Server actions only handle data operations (Select, Search, etc.) - Multi-select uses lvt-el:addClass:on:done="open" to stay open Removed: Toggle(), Close(), Show(), Hide() methods and Open bool fields from dropdown, menu, popover, drawer, datepicker, and timepicker. Kept: autocomplete Open field (data-dependent visibility). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… syntax BREAKING CHANGE: All component, generator, and kit templates updated: - lvt-click on buttons → name (Tier 1 button routing) - lvt-click on non-buttons → lvt-on:click (Tier 2 generic router) - lvt-data-* → data-* (standard HTML) - lvt-input → lvt-on:input, lvt-keydown → lvt-on:keydown, etc. - lvt-change → lvt-on:change - lvt-debounce → lvt-mod:debounce - lvt-submit → form/button name routing - lvt-modal-open/close → command/commandfor (native dialog) - lvt-confirm → onclick="return confirm(...)" - lvt-disable-with → lvt-form:disable-with - JS querySelector selectors updated for new attribute names Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all chromedp selectors and inline HTML templates in e2e tests:
- button[lvt-click="X"] → button[name="X"]
- form[lvt-submit="X"] → form[name="X"]
- [lvt-modal-open="X"] → [commandfor="X"]
- lvt-modal-close → command="close" commandfor
- getAttribute('lvt-data-id') → getAttribute('data-id')
- Inline HTML: lvt-click → name, lvt-data-* → data-*
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewThis is a well-scoped, coherent refactor. The architectural direction (client-side open/close, standard HTML attributes) is sound and the mechanical renaming is thorough. A few things worth addressing: Bug:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates LiveTemplate templates and tests to the new attribute syntax while refactoring several UI components to manage open/close entirely client-side (removing server-managed Open state and related server methods).
Changes:
- Migrated templates from deprecated
lvt-*attributes toname,lvt-on:{event},lvt-mod:*, andlvt-form:*conventions across generators, kits, and components. - Refactored open/close behavior for multiple components (dropdown/menu/popover/drawer/datepicker/timepicker/autocomplete) to rely on client-side class toggling rather than server state.
- Updated E2E and parity tests to use new selectors/attributes.
Reviewed changes
Copilot reviewed 108 out of 108 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/golden/resource_template.tmpl.golden | Golden output updated for new attributes |
| internal/kits/system/single/templates/resource/template.tmpl.tmpl | Single kit resource template migration |
| internal/kits/system/single/templates/resource/embedded_template.tmpl.tmpl | Embedded resource template migration |
| internal/kits/system/single/components/toolbar.tmpl | Toolbar attribute migration |
| internal/kits/system/single/components/table.tmpl | Table action routing migration |
| internal/kits/system/single/components/sort.tmpl | Sort control migration |
| internal/kits/system/single/components/search.tmpl | Search control migration |
| internal/kits/system/single/components/pagination.tmpl | Pagination routing migration |
| internal/kits/system/single/components/layout.tmpl | Layout JS selector updates |
| internal/kits/system/single/components/form.tmpl | Form submission/routing migration |
| internal/kits/system/single/components/detail.tmpl | Detail actions migration |
| internal/kits/system/simple/templates/app/index.tmpl.tmpl | Simple kit button routing migration |
| internal/kits/system/simple/components/layout.tmpl | Layout JS selector updates |
| internal/kits/system/multi/templates/resource/template.tmpl.tmpl | Multi kit resource template migration |
| internal/kits/system/multi/templates/resource/embedded_template.tmpl.tmpl | Embedded resource template migration |
| internal/kits/system/multi/templates/auth/template.tmpl.tmpl | Auth template migration |
| internal/kits/system/multi/templates/auth/e2e_test.go.tmpl | Auth E2E template selector updates |
| internal/kits/system/multi/components/toolbar.tmpl | Toolbar attribute migration |
| internal/kits/system/multi/components/table.tmpl | Table action routing migration |
| internal/kits/system/multi/components/sort.tmpl | Sort control migration |
| internal/kits/system/multi/components/search.tmpl | Search control migration |
| internal/kits/system/multi/components/pagination.tmpl | Pagination routing migration |
| internal/kits/system/multi/components/layout.tmpl | Layout JS selector updates |
| internal/kits/system/multi/components/form.tmpl | Form submission/routing migration |
| internal/kits/system/multi/components/detail.tmpl | Detail actions migration |
| internal/kits/kits_parity_test.go | Parity patterns updated for new attrs |
| internal/generator/templates/resource/template.tmpl.tmpl | Resource generator template migration |
| internal/generator/templates/components/toolbar.tmpl | Generator toolbar migration |
| internal/generator/templates/components/table.tmpl | Generator table migration |
| internal/generator/templates/components/sort.tmpl | Generator sort migration |
| internal/generator/templates/components/search.tmpl | Generator search migration |
| internal/generator/templates/components/pagination.tmpl | Generator pagination migration |
| internal/generator/templates/components/layout.tmpl | Generator layout JS selector update |
| internal/generator/templates/components/form.tmpl | Generator form routing migration |
| internal/generator/templates/components/detail.tmpl | Generator detail routing migration |
| internal/generator/embedded_resource_test.go | Embedded generation assertions updated |
| e2e/wire_format_test.go | Wire-format HTML updated to new attrs |
| e2e/tutorial_test.go | Tutorial E2E selectors updated |
| e2e/resource_generation_test.go | Resource-gen E2E assertions updated |
| e2e/rendering_test.go | Rendering E2E updated for new modal attrs |
| e2e/pagemode_test.go | Page-mode E2E selectors updated |
| e2e/modal_test.go | Modal E2E HTML updated for new attrs |
| e2e/livetemplate_core_test.go | Core E2E HTML updated for new attrs |
| e2e/embedded_browser_test.go | Embedded-browser E2E selectors updated |
| e2e/editmode_test.go | Edit-mode regex/assertions updated |
| e2e/edit_modal_reopen_test.go | Edit-modal E2E selectors updated |
| e2e/delete_multi_post_test.go | Delete-multi-post E2E selectors updated |
| e2e/complete_workflow_test.go | Full workflow E2E selectors updated |
| components/tooltip/templates/default.tmpl | Tooltip event attr migration |
| components/toggle/toggle.go | Toggle docs updated for new attrs |
| components/toggle/templates/default.tmpl | Toggle template event attr migration |
| components/toggle/templates/checkbox.tmpl | Toggle checkbox template migration |
| components/toast/toast.go | Toast docs updated for new routing |
| components/timepicker/timepicker.go | Timepicker server open/close removal |
| components/timepicker/timepicker_test.go | Timepicker tests updated/removed for Open |
| components/timepicker/templates/duration.tmpl | Duration picker template client open/close |
| components/timepicker/templates/default.tmpl | Time picker template client open/close |
| components/timepicker/options.go | WithOpen made no-op (compat) |
| components/tagsinput/templates/default.tmpl | Tagsinput event/routing migration |
| components/tagsinput/tagsinput.go | Tagsinput docs updated |
| components/tagsinput/tagsinput_test.go | Tagsinput template assertions updated |
| components/tabs/templates/vertical.tmpl | Tabs routing migration |
| components/tabs/templates/pills.tmpl | Tabs routing migration |
| components/tabs/templates/horizontal.tmpl | Tabs routing migration |
| components/tabs/tabs.go | Tabs docs updated |
| components/tabs/tabs_test.go | Tabs template assertions updated |
| components/rating/templates/default.tmpl | Rating event attr migration |
| components/rating/rating.go | Rating docs updated |
| components/popover/templates/default.tmpl | Popover client open/close migration |
| components/popover/popover.go | Popover server Open removed + docs |
| components/popover/popover_test.go | Popover tests updated for removed Open |
| components/popover/options.go | WithOpen made no-op (compat) |
| components/modal/templates/sheet.tmpl | Modal sheet template routing migration |
| components/modal/templates/default.tmpl | Modal template routing migration |
| components/modal/templates/confirm.tmpl | Confirm modal button routing migration |
| components/modal/modal.go | Modal docs updated re client open/close |
| components/menu/templates/nav.tmpl | Nav menu open/close client-side migration |
| components/menu/templates/default.tmpl | Menu open/close client-side migration |
| components/menu/templates/context.tmpl | Context menu template updated for new attrs |
| components/menu/options.go | WithOpen made no-op (compat) |
| components/menu/menu.go | Menu server Open removed + method changes |
| components/menu/menu_test.go | Menu tests updated for removed Open |
| components/dropdown/templates/searchable.tmpl | Searchable dropdown migration + open class |
| components/dropdown/templates/multi.tmpl | Multi dropdown migration + client open/close |
| components/dropdown/templates/default.tmpl | Default dropdown migration + client open/close |
| components/dropdown/options.go | WithOpen deprecated/no-op |
| components/dropdown/dropdown.go | Dropdown server Open removal + searchable Open |
| components/dropdown/dropdown_test.go | Dropdown tests updated for new behavior |
| components/drawer/templates/default.tmpl | Drawer client open/close migration |
| components/drawer/options.go | WithOpen deprecated/no-op |
| components/drawer/drawer.go | Drawer Open removed + transform refactor |
| components/drawer/drawer_test.go | Drawer tests updated for new transform |
| components/datepicker/templates/single.tmpl | Datepicker single template migration |
| components/datepicker/templates/range.tmpl | Datepicker range template migration |
| components/datepicker/options.go | WithOpen made no-op (compat) |
| components/datepicker/datepicker.go | Datepicker Open removed + methods removed |
| components/datepicker/datepicker_test.go | Datepicker tests updated for removed Open |
| components/datatable/templates/default.tmpl | Datatable attribute/routing migration |
| components/datatable/datatable.go | Datatable docs updated |
| components/base/action.go | Data-* doc updates (lvt-data-* → data-*) |
| components/autocomplete/templates/multi.tmpl | Autocomplete multi template migration |
| components/autocomplete/templates/default.tmpl | Autocomplete default template migration |
| components/autocomplete/autocomplete.go | Autocomplete Focus/Blur methods removed |
| components/autocomplete/autocomplete_test.go | Autocomplete tests updated for removals |
| components/accordion/templates/single.tmpl | Accordion routing migration |
| components/accordion/templates/default.tmpl | Accordion routing migration |
| components/accordion/accordion.go | Accordion docs updated |
| components/accordion/accordion_test.go | Accordion template assertions updated |
| <div | ||
| class="{{.Styles.Dropdown}}" | ||
| lvt-click-away="close_{{.ID}}" | ||
| data-timepicker-panel | ||
| hidden | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="Choose time" |
There was a problem hiding this comment.
data-timepicker-panel is always rendered with the hidden attribute, but the open/close logic now only toggles the .open class. Since there is no code removing hidden (and the CSS only adds a rule for the closed state), the panel will remain non-rendered even when .open is present. Remove the hidden attribute or toggle it in the same handler that toggles the .open class.
| <div | ||
| class="{{.DurationStyles.Dropdown}}" | ||
| lvt-click-away="close_{{.ID}}" | ||
| data-timepicker-panel | ||
| hidden | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="Choose duration" |
There was a problem hiding this comment.
Same issue as the default timepicker: the duration panel includes hidden but open/close is now driven by toggling the .open class only, so the panel will never become visible. Remove hidden or ensure it is toggled alongside the .open class.
| class="{{.Styles.Dropdown}}" | ||
| lvt-click-away="close_datepicker_{{.ID}}" | ||
| data-datepicker-panel | ||
| hidden |
There was a problem hiding this comment.
The datepicker dropdown is rendered with the hidden attribute, but the trigger now only toggles the .open class on the root. Without also removing/toggling hidden, the panel will stay display:none even when open. Either remove hidden and rely on the .open CSS, or update the click handler to manage hidden as well.
| hidden |
| class="{{.Styles.Dropdown}}" | ||
| lvt-click-away="close_datepicker_{{.ID}}" | ||
| data-datepicker-panel | ||
| hidden |
There was a problem hiding this comment.
The range datepicker dropdown is always rendered with hidden, but visibility is now controlled via .open class toggling. Since hidden is never removed/toggled, the panel will not appear. Remove hidden or toggle it when opening/closing.
| hidden |
| {{if .IsFocusTrigger}} | ||
| lvt-focus="show_{{.ID}}" | ||
| lvt-blur="hide_{{.ID}}" | ||
| onfocus="this.closest('[data-popover]').classList.add('open')" | ||
| onblur="this.closest('[data-popover]').classList.remove('open')" | ||
| {{end}} |
There was a problem hiding this comment.
Using onfocus/onblur on this wrapper <div> will not reliably open/close the popover when a descendant (the actual trigger element) receives focus, because native focus/blur do not bubble. Use onfocusin/onfocusout (which do bubble) or move the handlers onto the actual focusable trigger element.
| // - New() creates a time picker (template: "lvt:timepicker:default:v1") | ||
| // - NewDuration() creates a duration picker (template: "lvt:timepicker:duration:v1") | ||
| // | ||
| // Required lvt-* attributes: lvt-click, lvt-input, lvt-click-away | ||
| // Required lvt-* attributes: lvt-click, lvt-input, lvt-el:removeClass:on:click-away, lvt-el:addClass:on:done | ||
| // |
There was a problem hiding this comment.
The top-level docs still list lvt-click/lvt-input as required attributes, but the templates now use inline onclick on the trigger and name="..." routing for the action buttons (plus lvt-el:* modifiers). Please update this comment to reflect the current required attributes/behavior so consumers aren’t misled.
e2e/rendering_test.go
Outdated
| <button id="open-modal" command="show-modal" commandfor="test-modal">Open Modal</button> | ||
|
|
||
| <div id="test-modal" hidden aria-hidden="true" role="dialog" data-modal-backdrop data-modal-id="test-modal" | ||
| style="position: fixed; top: 0; left: 0; width: 100%; height: 100%; background: rgba(0,0,0,0.5); display: flex; align-items: center; justify-content: center; z-index: 1000;"> | ||
| <div style="background: white; padding: 2rem; border-radius: 8px;"> | ||
| <h2>Test Modal</h2> | ||
| <p id="modal-content">Modal content here</p> | ||
| <button id="close-modal" lvt-modal-close="test-modal">Close</button> | ||
| <button id="close-modal" command="close" commandfor="test-modal">Close</button> |
There was a problem hiding this comment.
PR description mentions migrating lvt-modal-open/close to command/commandfor as a native dialog mechanism, but this test (and the embedded markup) still uses a <div role="dialog"> with hidden + style.display === 'flex' assertions. If the intent is native <dialog>, the markup/assertions should be updated; if the intent is a LiveTemplate-specific command router for non-<dialog> elements, the PR description should be clarified to avoid implying browser-native dialog behavior.
…ardize docs - Extract shared panel CSS rules into _shared.tmpl for dropdown, autocomplete, timepicker, and datepicker (deduplicates across variants) - Remove stale `hidden` attr from datepicker/timepicker panels that conflicted with CSS visibility pattern (bug: panels wouldn't show) - Standardize WithOpen() deprecated doc comments across all components - Remove duplicate "client-side" comments from struct-level docs - Fix stale doc comments in timepicker.go and tooltip.go - Remove orphan "methods removed" comments from test files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewThis is a well-executed, internally consistent refactor. The mechanical renaming is thorough and the diff is a net simplification (~660 additions vs. 1156 deletions). A few concerns worth addressing: Medium:
|
- Use onfocusin/onfocusout instead of onfocus/onblur on popover wrapper (focus/blur don't bubble to parent elements) - Fix timepicker.go doc comment (was listing HTML attrs as lvt-* attrs) - Update NewInline() doc to note consuming template must add "open" class Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review (additive)The refactor is well-executed. Two prior reviews cover the main concerns well — this adds two issues not yet mentioned. Bug: Autocomplete panel stays open after suggestion selectionPreviously, selecting a suggestion called With the refactor,
Result: after clicking a suggestion, the (now-empty) suggestion list stays visible until the user clicks outside. The same applies to keyboard selection via Fix options:
UX regression: No focusout handler in autocompletePreviously A keyboard user tabbing out of the autocomplete input will leave the suggestion dropdown open. Fix: add onfocusout="if(!this.closest('[data-autocomplete]').contains(event.relatedTarget))this.closest('[data-autocomplete]').classList.remove('open')"Everything else in the prior reviews still stands. |
Without onfocusout, tabbing away from the autocomplete input left the suggestion dropdown open (click-away only fires on mouse clicks, not keyboard navigation). The handler guards against focus moving into the suggestion list itself. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: feat! — Attribute Reduction, Client-Side Open/Close + Template MigrationThis is a well-structured, coherent breaking change with a clear architectural direction. The attribute renaming is thorough, internally consistent, and the test coverage updates are complete for unit tests. Several issues are worth addressing before merging. Bugs / Correctness Issues1. Persistent drawer The old onclick="var d=this.closest('[data-drawer]');if(!d.hasAttribute('data-persistent'))d.classList.remove('open')"This inline handler does preserve the persistent check — but only for the close button and overlay. There is no replacement for 2. All toggled components now hardcode 3. Searchable dropdown The // Open is server-controlled for searchable dropdowns because visibility
// depends on whether the query meets MinChars and results exist.
Open boolThis is a principled design choice, but the template renders the panel unconditionally (no 4.
5. Context menu
// ShowAt sets the context menu position.
// The consuming page is responsible for adding the "open" CSS class to show it.
func (cm *ContextMenu) ShowAt(x, y int) {
cm.X = x
cm.Y = y
}Any existing application code calling Design / API Concerns6.
7. Inline event handlers and Content-Security-Policy The migration from 8. Duplicate Each template now injects its own <style>[data-dropdown]:not(.open) [data-dropdown-panel]{display:none}</style>A page with three dropdowns will have three identical 9. The old Code Quality10. Toggle icon CSS class lost for menu/dropdown The trigger button SVG chevron previously applied an open-state style: <svg class="{{$c.Styles.TriggerIcon}} {{if $c.Open}}{{$c.Styles.TriggerIconOpen}}{{end}}"Now it's unconditional: <svg class="{{$c.Styles.TriggerIcon}}"The chevron no longer rotates to indicate open state via server-side class. If the styled kit previously used a Tailwind class like Test Coverage11. E2E tests gated on unpublished client The test plan notes E2E browser tests require published client v0.8.17+. The 12. No test for The existing Positive Observations
SummaryThe architectural direction is sound and the mechanical renaming is thorough. Before merging, the most important items to address are:
🤖 Generated with Claude Code |
Replace onfocus/onfocusout/onmouseenter/onmouseleave with declarative lvt-el: attributes on autocomplete, searchable dropdown, and popover. Trigger buttons that toggle parent open state keep onclick for now — lvt-el: operates on self, not parent elements. A future client library enhancement (target selector) will address this. Requires: @livetemplate/client with DOM event trigger support (PR #49) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review — PR #292: Attribute Reduction, Client-Side Open/Close + Template MigrationThis is a well-executed, architecturally sound breaking change. The mechanical renaming is thorough and consistent across all 108 changed files, and the net diff (-1161/+669 lines) shows real simplification. Several issues need to be addressed before merging. Bugs1. NewInline() datepicker no longer renders the calendar visible NewInline() previously set Open = true. With Open removed and the panel hidden via [data-datepicker]:not(.open) [data-datepicker-panel]{display:none}, NewInline() now renders with the calendar hidden. No code path sets the open class on the root element for inline instances. The docstring says "The consuming template should add the 'open' class" but the built-in template does not do this. Fix: add open to the root element's class unconditionally in the inline template variant, or add an AlwaysOpen bool field the template checks. The deleted TestNewInline (which verified Open=true) was replaced with a no-panic smoke test, leaving the behavior change untested. 2. Autocomplete panel does not close after suggestion selection Previously Select() set Open = false. Now Open is removed and the panel is controlled by the client-side open CSS class, but nothing removes that class after selection — lvt-on:click="select_ID" triggers a server re-render that cannot modify client-side CSS class state, and there is no lvt-el:removeClass:on:done="open" on the autocomplete root. After clicking a suggestion, the empty list will remain open until the user clicks outside. This is a functional regression. Fix: add lvt-el:removeClass:on:done="open" to the root div in both default.tmpl and multi.tmpl. 3. ContextMenu.ShowAt() no longer opens the context menu ShowAt(x, y) previously set coordinates and set Open = true. Now it only sets coordinates. Application code calling ShowAt() from an action handler will update the position but the menu will silently remain closed. Suggest adding Open bool back on ContextMenu specifically (the only component where server-side show/hide is necessary at action time), or documenting the expected pattern with a working example. 4. Tab-away closes autocomplete while user is selecting a suggestion lvt-el:removeClass:on:focusout="open" fires whenever focus leaves any element inside the component — including when focus moves from the input to a suggestion item via keyboard Tab. This incorrectly closes the dropdown mid-selection. The old Blur() approach handled this implicitly. Consider onfocusout with a relatedTarget check, or a small delay before hiding. Silent Breaking Changes5. WithOpen(true) is now a no-op with no migration path Application code that used WithOpen(true) to initialize a component open will silently stop working. The deprecation comments say "has no effect" but provide no migration path. Consider documenting how to achieve pre-opened state via CSS class injection, or provide a WithOpenClass() Option. 6. Inline onclick handlers break strict Content-Security-Policy The migration from lvt-click="toggle_X" to onclick="var r=this.closest(...)..." requires 'unsafe-inline' in script-src. The old lvt-* attributes were CSP-safe. This is a regression for applications with strict CSP. At minimum, note it in the CHANGELOG. Ideally, encode toggle behavior as a new framework directive (e.g., lvt-toggle:class="open") consistent with the existing lvt-el:* syntax. Accessibility7. aria-expanded resets to "false" on every server re-render All trigger buttons now hardcode aria-expanded="false", relying on inline JS to flip it. Any server-triggered partial re-render will return aria-expanded="false" even if the panel is visually open. Screen readers will report the control as collapsed when it is expanded. Affects: dropdown/default.tmpl, dropdown/searchable.tmpl, dropdown/multi.tmpl, menu/default.tmpl, menu/nav.tmpl, datepicker/single.tmpl, datepicker/range.tmpl, timepicker/default.tmpl, timepicker/duration.tmpl, popover/default.tmpl. Design / Consistency8. lvt-el:addClass:on:done="open" inconsistency across picker variants datepicker/range.tmpl, timepicker/default.tmpl, and timepicker/duration.tmpl include this directive, but datepicker/single.tmpl does not. Is the omission intentional (close-on-select UX)? A comment explaining the difference would clarify intent. 9. Chevron indicator may be lost for styled kits The trigger SVG previously used {{if $c.Open}}{{$c.Styles.TriggerIconOpen}}{{end}}. Now it is unconditional. If a kit uses rotate-180 for TriggerIconOpen, that visual indicator is lost unless kit CSS has a parent .open selector rule. Please verify kit CSS covers this. 10. Duplicate style blocks per component instance Each component template emits its own style block via _shared.tmpl. Three dropdown instances produce three identical blocks. Browsers handle this correctly but it is unnecessary HTML weight — worth documenting as a known trade-off. Code Quality11. Stale docstring in timepicker.go The "Required lvt-* attributes" comment still lists lvt-click and lvt-input, but the PR replaces lvt-click with onclick="..." and updates to lvt-on:input. Should be updated. 12. Searchable.Open interaction with lvt-el:addClass:on:focusin The interaction between the server-controlled Open field and the lvt-el:addClass:on:focusin directive should be documented or tested more explicitly. If Open is false but a search has results, the panel will not show. Test Coverage13. E2E tests blocked on unpublished client The test plan requires client v0.8.17+. The new lvt-on:* routing, lvt-el:* directives, and command/commandfor modal attributes are not validated by any currently-running tests. A follow-up issue should track the v0.8.17+ publish gate before this PR is fully validated. 14. NewInline() rendering is untested (see Bug #1) Positive Observations
SummaryCritical items before merging:
Items #7 (aria-expanded), #4 (tab-away), and #5 (WithOpen migration path) are worth tracking as follow-ups. Reviewed by Claude Sonnet 4.6 |
…ck toggles - Replace command="show-modal"/command="close" commandfor="X" with lvt-el:toggleAttr:on:click="hidden" data-lvt-target="#X" in all generator, kit, and test templates (23 occurrences) - Replace onclick toggle handlers on dropdown, popover, datepicker, timepicker, and menu components with lvt-el:toggleClass:on:click="open" data-lvt-target="closest:[data-X]" (10 component templates) - Update e2e test selectors from [commandfor="..."] to [data-lvt-target="..."] - Update golden files and unit test assertions - Remaining onclick: search clear (multi-action), confirm delete (browser dialog), drawer (conditional data-persistent check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review — Confirmed Issues SummaryThe prior review comments on this PR have been thorough. After reviewing the diff directly, here is a confirmed, prioritized summary of items that should be resolved before merging. Must Fix (Bugs / Regressions)1.
2. Autocomplete panel stays open after suggestion selection
3.
4. Stale docstring in Still lists Should Address5. All toggled components hardcode 6. CSP regression from inline The old 7. Application code using Minor
Positive Observations
Reviewed by Claude Sonnet 4.6 |
…play
With toggleAttr on hidden, the inline style.display stays "flex" but
the element is hidden via UA stylesheet. Tests now check hasAttribute('hidden')
instead of style.display for modal open/close verification.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: feat!: attribute reduction — client-side open/close + template migrationThis is a well-motivated refactor that eliminates server round-trips for purely visual state. The breadth of coverage (76 template files, 12 e2e test files) is impressive. Here are my observations: Bug:
|
Each chromedp.ActionFunc is a separate closure — var hidden bool must be declared at function level to be shared across all action closures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewThis is a well-scoped, architecturally sound PR. Moving UI-only open/close state from the server to client-side CSS class toggling eliminates unnecessary round-trips and the attribute naming migration makes the framework more idiomatic. The breadth of coverage — component Go files, tests, templates, generator templates, kit templates, and e2e tests — is impressive for a single PR. Potential Bug: Autocomplete focusin/focusout race conditionSeverity: Medium
<div ... lvt-el:addClass:on:focusin="open" lvt-el:removeClass:on:focusout="open">On the web platform, when a user clicks a suggestion, the sequence is: This is a classic focus-based visibility issue. Common solutions:
Worth verifying this doesn't break item selection, especially across browsers. Accessibility:
|
… dialog
Simplify 3-step server flow (request_delete → show modal → confirm_delete
→ delete) to 1-step: name="delete" onclick="return confirm('...')" with
data-id. The browser native confirm dialog handles "are you sure?" and
the delete routes directly to the server.
Changes:
- Merge RequestDelete + ConfirmDelete + CancelDelete into single Delete
handler that takes ID directly from BindAndValidate
- Remove DeleteConfirm modal state and PendingDeleteID from state struct
- Remove confirm modal initialization and template rendering
- Update delete buttons: request_delete → delete with onclick confirm
- Update layout JS routing: confirm_delete → delete
- Update golden files and test assertions
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review: feat! — attribute reduction, client-side open/close + template migrationThis is a large, well-scoped refactor with a clear architectural rationale. The changes are consistently applied across the codebase and the test suite follows the code changes faithfully. Overall this is a solid PR, with a few items worth discussing before merge. Architecture / DesignPositive: The core premise — eliminate server round-trips for purely UI-open/close state — is sound. Moving dropdown/menu/popover/drawer/datepicker/timepicker/autocomplete panel visibility to CSS class toggling reduces WebSocket traffic for operations that carry no server-side data. Concern: Persistent drawer no longer enforced server-side. onclick="var d=this.closest('[data-drawer]');if(!d.hasAttribute('data-persistent'))d.classList.remove('open')"This logic now lives in two separate inline handlers (overlay + close button). Any third-party code that drives the drawer via a Concern: // ShowAt sets the context menu position.
// The consuming page is responsible for adding the "open" CSS class to show it.
func (cm *ContextMenu) ShowAt(x, y int) {
cm.X = x
cm.Y = y
}Any user handler that called only Template ConsistencyMinor: Minor: Deprecated OptionsThe pattern used for deprecated // Deprecated: This option has no effect.
func WithOpen(_ bool) Option {
return func(_ *Drawer) {}
}Consider whether a Inline JavaScript PatternsThe drawer template introduces inline JS for close-on-overlay and close-button clicks: onclick="var d=this.closest('[data-drawer]');if(!d.hasAttribute('data-persistent'))d.classList.remove('open')"This is duplicated verbatim in two places within the same template (overlay div and close button). If the logic ever needs to change (e.g., dispatching a custom event on close), it must be updated in two places. Extracting to a named helper or a small inline Generator template: delete action changed silentlyIn lvt-click="request_delete" lvt-data-id="{{.EditingID}}"to: name="delete" data-id="{{.EditingID}}" onclick="return confirm(...)"And Test Coverage
Minor nits
Summary
The two breaking behavior changes ( 🤖 Generated with Claude Code |
Replace multi-step request_delete → confirm modal → confirm_delete flow with single delete button click. In headless Chrome, window.confirm() auto-returns true, so the browser confirm dialog is transparent to tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review:
|
| Area | Status |
|---|---|
| Architecture / direction | ✅ Sound |
| Attribute syntax migration | ✅ Consistent |
| Server round-trip elimination | ✅ Genuine improvement |
aria-expanded / accessibility |
❌ Regression — should fix before merge |
NewInline datepicker contract |
|
| Multi-dropdown reopen behavior | |
| Drawer inline JS | lvt-el: pattern |
| E2E tests passing | ❌ Blocked on client v0.8.17+ |
The execution is disciplined and the scope is well-managed. The aria-expanded regression and the E2E test gap are the two items I'd want resolved before this lands on main.
…e test - Override window.confirm = () => true before clicking delete buttons in headless Chrome (confirm() blocks JS execution in some drivers) - Remove Escape key test from standalone modal test (requires active WebSocket for client event delegation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewThis is a well-scoped architectural improvement — moving UI-only visibility state to the client eliminates unnecessary server round-trips and simplifies the server-side component model. The attribute rename is systematic and consistent across 124 files. A few issues worth addressing before merging: Bugs1. In lvt-el:addClass:on:focusin="open"
lvt-el:removeClass:on:focusout="open"When a user clicks a suggestion 2.
aria-expanded="{{$c.Open}}"
3. The removed if len(ac.Query) >= ac.MinChars && len(ac.FilteredSuggestions) > 0 {
ac.Open = true
}The replacement 4.
API / UX5. For popover, drawer, datepicker, etc.: // Deprecated: This option has no effect.
func WithOpen(_ bool) Option {
return func(_ *Popover) {}
}Any existing code calling Maintainability6. Repeated inline JS in drawer templates The same inline handler appears multiple times: onclick="var d=this.closest('[data-drawer]');if(!d.hasAttribute('data-persistent'))d.classList.remove('open')"This is repeated for the overlay click, the close button, and potentially other triggers. Any logic change requires hunting down every copy. Consider centralizing this in the client library as a named action (e.g., Testing7. E2E tests are not passing The PR description explicitly notes Positives
|
…tests - Delete buttons with type="button" inside forms use lvt-on:click="delete" instead of name="delete" (orphan button detection requires btn.form=null) - Remove rapid cycle and escape key tests from standalone modal test (WebSocket disconnects in standalone page; covered by full e2e tests) - Update golden files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewThis is a well-scoped, coherent refactor. Moving open/close state client-side and standardizing on Bug:
|
In-form delete buttons now use lvt-on:click="delete" instead of name="delete". E2e selectors updated to match both patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: feat! attribute reduction — client-side open/close + template migrationThis is a large, well-scoped refactor. The direction is sound — moving UI-only open/close state to client-side CSS eliminates unnecessary server round-trips, and the attribute rename aligns the API around standard HTML conventions. The diff is impressively balanced (766 additions vs 2067 deletions). A few issues are worth addressing before merge. Critical1. Inline
onclick="var d=this.closest('[data-drawer]');if(!d.hasAttribute('data-persistent'))d.classList.remove('open')"This inline JavaScript will fail under any CSP that doesn't include 2. Trigger buttons in dropdown, menu, datepicker, timepicker, and popover previously had Behavioral Breaking Changes (need docs or migration notes)3.
4.
5. There's no longer any way to render these components in their open state on initial server render. This matters for SSR scenarios like "open this dropdown by default" or "show the menu pre-expanded on page load." The deprecated Mixed Model / Potential Race Condition6. Autocomplete has a conflicting dual open/close mechanism The autocomplete component keeps a server-side These two mechanisms can race: Minor / Nits7. PR description inaccurate about modal migration The description says 8. Stale old-syntax fixture in The test fixture still uses What's Good
Summary: The CSP regression (#1) and accessibility regression (#2) should be resolved before merge. Items #3–#5 need at minimum clear migration docs. The autocomplete race condition (#6) is worth investigating. Everything else is minor. |
Go string \\: → JS string \: → CSS escaped colon in attribute name. Single backslash was producing unescaped colon interpreted as pseudo-class. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review — PR #292: Attribute Reduction (Client-Side Open/Close + Template Migration)This is a substantial, well-scoped refactor with +766/-2067. The overall direction is sound — removing server round-trips for purely visual state is a clear win. The migration is thorough and consistent. Below are observations, concerns, and questions worth addressing. Architecture / DesignPositive: The open/close client-side shift is architecturally correct. Concern: Concern: Concern: Template / HTML IssuesIssue: Modal
Accessibility: SecurityNote:
This is an acceptable trade-off in many contexts, but the behavior change should be documented: the previous two-step (request → confirm modal → delete) flow rendered the confirmation from trusted server HTML. The new single-step flow with a browser dialog is weaker. Persistent Drawer RegressionConcern: This will remove Inline
|
type="button" with name="X" inside a <form> is not caught by orphan button detection (btn.form !== null). Use lvt-on:click="X" instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review:
|
| Category | Finding | Severity |
|---|---|---|
| Bug | hidden attr on datepicker single panel prevents display |
High |
| Bug | lvt-el:addClass:on:done="open" reopens on any server response |
Medium |
| Accessibility | aria-expanded always resets to false on re-render |
Medium |
| Security | Inline onclick in drawer breaks strict CSP |
Medium |
| Docs | Stale docstring in timepicker.go |
Low |
| Performance | Duplicate <style> per instance |
Low |
The overall direction is correct and the migration is thorough. Fixing the hidden attribute bug and the CSP issue in drawer are the blockers I'd want addressed before merge. The aria-expanded issue is worth tracking as a follow-up issue if not addressed here.
Summary
Openstate to client-side CSS class toggling. Eliminates server round-trips for UI-only concerns.lvt-on:{event},lvt-mod:,lvt-form:syntax. 76 template files, perfectly balanced rename.Key changes
lvt-clickon buttons →name(Tier 1 routing)lvt-clickon non-buttons →lvt-on:click(Tier 2 generic router)lvt-data-*→data-*(standard HTML)lvt-submit→ form/buttonnameroutinglvt-modal-open/close→command/commandfor(native dialog)lvt-confirm→onclick="return confirm(...)"lvt-debounce→lvt-mod:debouncelvt-disable-with→lvt-form:disable-withlvt-click-away→lvt-el:removeClass:on:click-away="open"(client-side CSS)Test plan
lvt-*attributes in any template🤖 Generated with Claude Code