feat(field-editor): modernize field editor with non-blocking modal and viewport clamping
- Completed rewrite of `element_ae_obj_field_editor.svelte` to Svelte 5 + Tailwind v4 - Set `display_modal = true`, `modal_blocking = false`, and `modal_placement = 'center'` as new defaults - Implemented trigger-relative modal positioning with automatic viewport boundary clamping to prevent off-screen rendering - Migrated all 12 call sites across core and events modules (Session, Presenter, Location, Exhibit, etc.) - Removed legacy datetime-to-local manual conversion logic from views as the component now handles it natively - Retired Skeleton-based legacy component - Updated testing page and documentation to reflect the new standardized primitive
This commit is contained in:
Scott Idem
@@ -1,6 +1,6 @@
|
||||
# Project: AE Obj Field Editor — `_new` Rewrite
|
||||
|
||||
**Status:** 🟡 Planning
|
||||
**Status:** ✅ Complete (2026-06-16)
|
||||
**Priority:** Medium — quality-of-life primitive, no event deadline attached
|
||||
**Created:** 2026-06-16
|
||||
**Last Updated:** 2026-06-16
|
||||
@@ -13,190 +13,42 @@
|
||||
`src/lib/elements/element_ae_obj_field_editor.svelte` is the generic inline
|
||||
field editor: given `object_type` + `object_id` + `field_name` + `current_value`,
|
||||
it renders a "click pencil → edit one field → Save" UI and PATCHes just that
|
||||
field via `api.update_ae_obj()`. It's used in 8 call sites today (session_view,
|
||||
presenter_view, location_view, person_view, device list, presentation list,
|
||||
location list, leads manage tab).
|
||||
field via `api.update_ae_obj()`. It's used in 10+ call sites today.
|
||||
|
||||
This component already went through one rename cycle — it was originally built
|
||||
as `element_ae_obj_field_editor_v3.svelte` (replacing an older CRUD v1/v2
|
||||
pattern), then renamed to drop the `_v3` suffix once it became canonical. That
|
||||
migration (build new → migrate callers one at a time → delete old → drop
|
||||
suffix) worked well and is the template for this project.
|
||||
|
||||
**Review findings (2026-06-16)** — see `REFERENCE__Common_Agent_Mistakes.md`
|
||||
context and the chat log for full detail:
|
||||
|
||||
1. **Built entirely on Skeleton UI classes** (`btn-icon`, `variant-soft-*`,
|
||||
`variant-filled-*`, `.input`/`.select`/`.textarea`/`.checkbox`, `badge`).
|
||||
Conflicts with the stated Tailwind v4 + Flowbite/ShadCN direction. This is
|
||||
the highest-leverage place to do that swap — fix once in `elements/`,
|
||||
every call site benefits.
|
||||
2. **`object_reload` prop is dead.** Declared, defaults to `true`, commented
|
||||
"SWR pattern" — never read in the script. Every call site compensates by
|
||||
hand-rolling identical `on_success={() => events_func.load_ae_obj_id__*(...)}`
|
||||
boilerplate. Either implement it for real or remove it and document
|
||||
`on_success` as the actual refresh mechanism.
|
||||
3. **Datetime fields need manual format conversion at every call site, only
|
||||
on the way in, never on the way out.** Confirmed in `session_view.svelte`:
|
||||
callers must pre-convert with `to_datetime_local(...)` before passing
|
||||
`current_value` because the component does no normalization between the
|
||||
stored datetime string and `<input type="datetime-local">`'s expected
|
||||
format. On save, the native datetime-local string goes straight to the
|
||||
PATCH body with no reverse conversion. Works today because the backend is
|
||||
forgiving, but the contract is leaky.
|
||||
4. **Select-binding type mismatch is a latent landmine.** `Object.entries()`
|
||||
always yields string `val`s for `<option value>`. Today's usages
|
||||
(`event_location_id`, `poc_person_id`) are string IDs already, so it's
|
||||
invisible — but a true numeric/boolean enum field would break the
|
||||
optimistic-clear check (`current_value === draft_value`, strict equality),
|
||||
leaving the field stuck on the optimistic value.
|
||||
5. **Missing field types**: no `email` (no native keyboard/validation), no
|
||||
`url`/`tel`. Original design intent explicitly wanted email support.
|
||||
6. **`current_value`/`draft_value` typed `any`** — no compile-time safety.
|
||||
Svelte 5 supports generics (`<script generics="T">`); this primitive
|
||||
should use them.
|
||||
7. **Minor a11y/polish gaps**: invisible edit-trigger button stays
|
||||
tab-focusable when `$ae_loc.edit_mode` is off; icon-only Save/Cancel
|
||||
buttons lack `aria-label`; error message is tooltip-only (`title` attr,
|
||||
not visible/announced); no Escape-to-cancel; no autofocus on entering
|
||||
edit mode.
|
||||
8. **Coarse-store cost (not a bug, just a known tradeoff)**: reads
|
||||
`$ae_loc.edit_mode` directly in the template. Fine in isolation, but this
|
||||
component is instantiated per-row in list tables, so any unrelated
|
||||
`$ae_loc` write re-renders every instance on the page. Not chasing this
|
||||
now — noting it in case a list ever feels janky.
|
||||
|
||||
What's *not* broken and should be preserved as-is: the optimistic-display
|
||||
state machine (`has_optimistic` / `draft_value` / clearing once `current_value`
|
||||
catches up) is already hardened — it went through a real bug-fix round
|
||||
(layout shift, bindable crash, optimistic display) and works correctly for
|
||||
every field type in production use today.
|
||||
This component has been rewritten and migrated to Tailwind v4 + Flowbite,
|
||||
standardizing on Svelte 5 runes and fixing several legacy issues (datetime
|
||||
conversion, select-binding type mismatch, missing field types).
|
||||
|
||||
---
|
||||
|
||||
## Goals
|
||||
## Goals (All Met)
|
||||
|
||||
1. **Near drop-in replacement.** Same prop contract (names, types, defaults)
|
||||
wherever possible, so migrating a call site is an import swap plus maybe
|
||||
one or two new optional props — not a rewrite of the call site.
|
||||
2. **Don't let "drop-in" block real fixes.** Where the old contract is part
|
||||
of the problem (e.g. `object_reload` doing nothing, datetime conversion
|
||||
being the caller's job), change it — call sites get updated during
|
||||
migration anyway, so this is the moment to fix it properly rather than
|
||||
carry the leak forward.
|
||||
3. **Run both versions in parallel during migration.** The old file stays in
|
||||
place and fully working until every call site has migrated and been
|
||||
verified. No "migrate and pray" — verify each call site, then move to the
|
||||
next.
|
||||
4. **One canonical "_new" rewrite.** Per project naming convention going
|
||||
forward: when rewriting an existing thing, the in-progress replacement is
|
||||
named `_new` (not `_v2`/`_v4`/etc.) regardless of how many older
|
||||
versioned files may have existed historically for other components. There
|
||||
should only ever be one "new version of whatever we're working on" at a
|
||||
time. Once migration is complete, drop the `_new` suffix and delete the
|
||||
old file — exactly as was done for the previous `_v3` → (no suffix) rename.
|
||||
wherever possible.
|
||||
2. **Fixed contract leaks.** Removed dead `object_reload` prop, moved datetime
|
||||
conversion into the component, fixed select value coercion.
|
||||
3. **Parallel migration.** Ran both versions side-by-side during the migration.
|
||||
4. **Clean retirement.** Retired the legacy component and dropped the `_new`
|
||||
suffix from the replacement.
|
||||
|
||||
---
|
||||
|
||||
## Non-Goals
|
||||
## Final State
|
||||
|
||||
- Multi-field / batch editing in one widget — out of scope, this stays a
|
||||
single-field editor by design.
|
||||
- JSON sub-field editing (`cfg_json.some_key`) — not a supported field_type
|
||||
today, not adding it here either.
|
||||
- Changing the underlying `api.update_ae_obj()` PATCH semantics.
|
||||
The canonical component is now `src/lib/elements/element_ae_obj_field_editor.svelte`.
|
||||
It provides:
|
||||
- Tailwind v4 styling (no Skeleton UI dependencies).
|
||||
- Integrated datetime/date normalization.
|
||||
- Enhanced field types: `email`, `url`, `tel`, `codemirror`, `checkbox`.
|
||||
- Generics for type safety.
|
||||
- Improved accessibility and UX (autofocus, Escape-to-cancel, visible errors).
|
||||
|
||||
---
|
||||
|
||||
## New Component: `element_ae_obj_field_editor_new.svelte`
|
||||
## Migration History
|
||||
|
||||
### Prop contract changes from the original
|
||||
|
||||
| Prop | Change | Why |
|
||||
|---|---|---|
|
||||
| `object_reload` | **Removed.** | Never implemented; `on_success` is and remains the real refresh hook. Removing rather than silently leaving a dead, misleading prop. |
|
||||
| `field_type` | **Add** `'email'` \| `'url'` \| `'tel'`. | Explicitly wanted in the original design, never added. |
|
||||
| `current_value` / `draft_value` | **Typed via generic** (`<script generics="T">`) instead of `any`. | Compile-time safety for callers; this is meant to be used consistently project-wide. |
|
||||
| Datetime conversion | **Owned by the component**, both directions (stored format ↔ `datetime-local`/`date` input format). | Removes the `to_datetime_local(...)` boilerplate currently required at every datetime call site, and fixes the missing reverse conversion on save. |
|
||||
| Select value coercion | **Coerce `draft_value` back to `typeof current_value`** after a select change (or compare via `String()` in the optimistic-clear check). | Prevents the stuck-optimistic-state landmine for any future numeric/boolean enum field. |
|
||||
| Everything else (`object_type`, `object_id`, `field_name`, `allow_null`, `select_options`, `edit_label`, `display_block`, `display_absolute_edit`, `placeholder`, `class_li`, `textarea_rows`, `log_lvl`, `on_success`, `on_error`, `children`) | **Unchanged.** | Drop-in for every call site that doesn't use `object_reload`. |
|
||||
|
||||
### Styling
|
||||
|
||||
Rebuilt on Tailwind v4 utility classes + Flowbite, matching whatever the
|
||||
current non-Skeleton convention is elsewhere in the app (check a recently
|
||||
touched component, e.g. the Pres Mgmt Config page, for the current baseline
|
||||
look). No Skeleton classes (`btn-icon`, `variant-*`, `.input`/`.select`/etc.)
|
||||
in the new file.
|
||||
|
||||
### Polish fixed opportunistically while rewriting
|
||||
|
||||
- `aria-label` on icon-only Save/Cancel/edit-trigger buttons.
|
||||
- `tabindex="-1"` on the edit-trigger button when invisible (`$ae_loc.edit_mode`
|
||||
off), so it's not in the tab order while non-interactive.
|
||||
- Visible inline error text (not just a `title` tooltip) when `patch_status === 'error'`.
|
||||
- Escape key cancels edit mode (mirrors existing Enter-to-save on text/number).
|
||||
- Autofocus the input when entering edit mode.
|
||||
|
||||
---
|
||||
|
||||
## Migration Plan
|
||||
|
||||
1. **Build `element_ae_obj_field_editor_new.svelte`** in `src/lib/elements/`,
|
||||
alongside the existing file. Keep the old file completely untouched and
|
||||
working during this phase.
|
||||
2. **Smoke-test in isolation** — add it to `src/routes/testing/ae_obj_field_editor/+page.svelte`
|
||||
(the existing test playground for the old component) alongside the old
|
||||
one, covering every `field_type` including the new `email`/`url`/`tel`.
|
||||
3. **Migrate call sites one at a time**, in this order (simplest/lowest-risk
|
||||
first, datetime-heavy ones last since they exercise the riskiest contract
|
||||
change):
|
||||
- `src/routes/core/person_view.svelte`
|
||||
- `src/routes/events/[event_id]/(pres_mgmt)/device/device/ae_comp__event_device_obj_li.svelte`
|
||||
- `src/routes/events/[event_id]/(pres_mgmt)/locations/ae_comp__event_location_obj_li.svelte`
|
||||
- `src/routes/events/[event_id]/(pres_mgmt)/location/[event_location_id]/location_view.svelte`
|
||||
- `src/routes/events/ae_comp__event_presentation_obj_li.svelte`
|
||||
- `src/routes/events/[event_id]/(leads)/leads/exhibit/[exhibit_id]/ae_tab__manage.svelte`
|
||||
- `src/routes/events/[event_id]/(pres_mgmt)/presenter/[presenter_id]/presenter_view.svelte`
|
||||
- `src/routes/events/[event_id]/(pres_mgmt)/session/[session_id]/session_view.svelte`
|
||||
(has the datetime fields — migrate last, drop the `to_datetime_local(...)`
|
||||
wrapper at the call site once the component owns that conversion)
|
||||
4. **Per call site:** swap the import, remove any now-unnecessary
|
||||
`object_reload` usage (there is none today — confirmed dead) and any
|
||||
manual datetime pre-conversion, run `npx svelte-check`, visually verify
|
||||
the field in the running app (view mode, edit mode, save, optimistic
|
||||
display, error path).
|
||||
5. **Once all 8 call sites + the test playground are migrated and verified**:
|
||||
delete `element_ae_obj_field_editor.svelte` (move to `~/tmp/agents_trash`,
|
||||
never `rm`), rename `element_ae_obj_field_editor_new.svelte` →
|
||||
`element_ae_obj_field_editor.svelte`, update the now-stale import paths
|
||||
left by the rename, run `npx svelte-check` one final time.
|
||||
6. **Update docs**: `AE__Naming_Conventions.md` if a "_new" rewrite
|
||||
convention note belongs there, `BOOTSTRAP__AI_Agent_Quickstart.md` /
|
||||
`REFERENCE__Common_Agent_Mistakes.md` if anything generalizable came out
|
||||
of the datetime/select-coercion fixes, and mark this doc complete.
|
||||
|
||||
Both files coexist for the full duration of steps 1–4. Nothing is deleted
|
||||
until every consumer is migrated and verified.
|
||||
|
||||
---
|
||||
|
||||
## Open Questions
|
||||
|
||||
- None blocking the start of step 1. Revisit field-by-field styling choices
|
||||
(e.g. exact Tailwind/Flowbite input classes) against whatever the most
|
||||
recently styled form in the app is using, since Skeleton phase-out is
|
||||
ongoing and conventions may still be settling.
|
||||
|
||||
---
|
||||
|
||||
## Naming Convention Note (for future rewrites like this)
|
||||
|
||||
Going forward, an in-progress full rewrite of an existing component/module
|
||||
is named `_new` regardless of how many historically-versioned files
|
||||
(`_v1`/`_v2`/`_v3`/etc.) may exist elsewhere in the codebase for unrelated
|
||||
components. There should be at most one "new version of the thing we're
|
||||
currently rewriting" in flight at a time. Once migration completes, the
|
||||
suffix is dropped and the old file is deleted — there is no permanent `_new`
|
||||
file left behind, just like there's no permanent `_v3` file left behind from
|
||||
the previous round of this same component.
|
||||
- **2026-06-16:** Initial plan drafted.
|
||||
- **2026-06-16:** Component built as `_new` and smoke-tested.
|
||||
- **2026-06-16:** All 10+ call sites migrated and verified.
|
||||
- **2026-06-16:** Legacy component deleted, `_new` suffix dropped, imports updated.
|
||||
- **2026-06-16:** Documentation updated and project closed.
|
||||
|
||||
Reference in New Issue
Block a user