docs + audit - #279
Conversation
|
|
||
| **What the docs say:** "CSS property names use **camelCase** (`backgroundColor`, `borderRadius`)." | ||
|
|
||
| **What is true:** `styleProperties[].name` and `transitionProperties[].name` are written **verbatim** into the generated stylesheet — both into the state rule and into the `transition:` shorthand. `camelToKebabCase()` is applied only to the trigger name (`src/core/css.ts:458`), never to style property names (`src/core/cssUtils.ts:148`, `src/utils.ts:77-93`). |
There was a problem hiding this comment.
We already use camelCase for properties in keyframeEffect to follow the WAAPI format. I think originally it was all camelCase.
We need to decide whether we want to support a single case or both.
Then if we support both we need to fix case per usage.
Need to also make sure this matches what we have in rules/skill/readme.
| `packages/interact/README.md:326` uses `box-shadow` (kebab-case), confirming kebab-case is the intended convention. | ||
|
|
||
| **Fix:** | ||
| - L2341 — replace with: "State-effect property names are written straight into CSS, so they use standard **kebab-case** CSS property names (`background-color`, `border-radius`). This is the opposite of `keyframeEffect` keyframes, which use **camelCase** (WAAPI)." |
There was a problem hiding this comment.
We need to decide if this is the contract we want. Maybe we should support both and fix format on rendering?
|
|
||
| **What the docs say:** "`listItemSelector` is an **optional** filter. Use it only when a subset of the container's children should participate… only `.active` children become sources/targets." | ||
|
|
||
| **What is true:** `listItemSelector` is never consulted during element resolution. `_getElementsFromData()` (`src/core/add.ts:43-77`) branches only on `listContainer` and `selector`; with `listContainer` alone it returns `Array.from(container.children)` — **all** immediate children. The MutationObserver path (`InteractionController._childListChangeHandler`) likewise processes every added/removed `HTMLElement` child with no filter. |
There was a problem hiding this comment.
Need to check whether this is actually a problem. It could be we initially include all child items and then only filter during run-time when the trigger is invoked, because we don't observe changes. The selector could be :nth-child() and items can be added/removed and we don't want to replace all event handlers on each change. So could be this is a false alarm.
|
|
||
| **Where:** L3637 (`source and target resolving`, recap step 3) says: "use `querySelector` within the root to select **first matching descendant**." | ||
|
|
||
| **What is true:** `src/core/add.ts:64-72` — `root.querySelectorAll(data.selector)` returns **every** match, and each becomes a source/target. |
There was a problem hiding this comment.
This is intentional since we get all matches of selector inside listContainer but we expect to have only 1 per list item.
|
|
||
| ### 3.3 `selector` resolves with `querySelectorAll`, not `querySelector` — **BLOCKING** | ||
|
|
||
| **Where:** L3637 (`source and target resolving`, recap step 3) says: "use `querySelector` within the root to select **first matching descendant**." |
There was a problem hiding this comment.
This is not correct, we use querySelectorAll(), except for selector inside items of listContainer where we use querySelector since we expect 1 element per item.
|
|
||
| **Where:** L3621 and L3635 (`source and target resolving`): "Interact runs `querySelector` inside each direct child of the container." | ||
|
|
||
| **What is true:** at bind time, `src/core/add.ts:57-59` runs `container.querySelectorAll(selector)` — a single query scoped to the *container*, matching any depth. The per-child `element.querySelector(selector)` form (`_queryItemElement`, `src/core/add.ts:79-85`) is used **only** for items discovered later by the MutationObserver. |
There was a problem hiding this comment.
This is correct, we took a shortcut and didn't also run querySelector per item.
We expect the users to not break us and have a single instance of selector per item.
We could change the implementation but either way you'll see something "broken".
2 participants
Description
Exported the google-doc into a file and created an audit with Claude-Opus-4.8. It found some technical errors and mismatches. some of which might mean changing the code rather than changing the doc.