Skip to content

docs: remove false animate-[...] arbitrary-value section (#83)#88

Closed
claude[bot] wants to merge 1 commit into
masterfrom
claude/issue-83-26483895483
Closed

docs: remove false animate-[...] arbitrary-value section (#83)#88
claude[bot] wants to merge 1 commit into
masterfrom
claude/issue-83-26483895483

Conversation

@claude
Copy link
Copy Markdown

@claude claude Bot commented May 27, 2026

Fixes #83

Removed the Arbitrary Values section from doc/interactivity/animation.md that claimed animate-[wiggle_1s_ease-in-out_infinite] works. AnimationParser only resolves theme-map keys via context.theme.animations.containsKey(); there is no bracket-syntax handler, so the documented example silently no-ops. Users wanting custom animations should use the Customizing Theme section directly below, which is the correct extension path.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes inaccurate documentation for animation arbitrary bracket syntax (animate-[...]) that is not implemented by the AnimationParser, and records the fix in the changelog to prevent users from relying on a no-op example.

Changes:

  • Removed the Arbitrary Values section (and its ToC entry) from doc/interactivity/animation.md that claimed animate-[...] works.
  • Added an [Unreleased] → Fixed entry to CHANGELOG.md documenting the docs correction and pointing users to theme customization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
doc/interactivity/animation.md Removes the incorrect animate-[...] arbitrary-value documentation and keeps the page structure/anchors coherent.
CHANGELOG.md Adds an Unreleased “Fixed” entry describing the docs correction and the correct extension path.

@sentry
Copy link
Copy Markdown

sentry Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread CHANGELOG.md

### Fixed

- `doc/interactivity/animation.md`: removed the Arbitrary Values section that claimed `animate-[...]` bracket syntax works; `AnimationParser` only resolves theme-map keys, so the example silently no-ops. Use the Customizing Theme section to register custom animations. (#83)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLAUDE.md rule: "one-line bullet per change." This reads as two sentences; the trailing sentence about the Customizing Theme section is guidance, not a description of the change itself. The issue ref (#83) is correct.

Suggested change
- `doc/interactivity/animation.md`: removed the Arbitrary Values section that claimed `animate-[...]` bracket syntax works; `AnimationParser` only resolves theme-map keys, so the example silently no-ops. Use the Customizing Theme section to register custom animations. (#83)
- `doc/interactivity/animation.md`: removed the `Arbitrary Values` section `animate-[...]` bracket syntax is not implemented in `AnimationParser`; tokens not present in `context.theme.animations` silently no-op. (#83)

```

<a name="customizing-theme"></a>
## Customizing Theme
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section (the PR's stated "correct extension path") has a bug in its own example.

AnimationParser looks up the full class token against the theme map — className is e.g. 'animate-wiggle', not 'wiggle':

// animation_parser.dart:40
context.theme.animations.containsKey(className)

The defaults confirm the key format (lib/src/theme/defaults/animations.dart):

'animate-spin': WindAnimationType.spin,
'animate-ping': WindAnimationType.ping,
// ...

The example below this heading uses 'wiggle' as the key. A user following that example gets the same silent no-op this PR is trying to fix. The key must be 'animate-wiggle':

// WRONG (current)
WindThemeData(animations: {'wiggle': WindAnimationType.bounce})

// CORRECT
WindThemeData(animations: {'animate-wiggle': WindAnimationType.bounce})

This needs fixing before the PR lands — otherwise removing the arbitrary-values section and pointing to this section is a net-zero improvement for users.

@claude
Copy link
Copy Markdown
Author

claude Bot commented May 27, 2026

Review: docs: remove false animate-[...] arbitrary-value section (#83)

Direction is correct; one blocker before merging.

The PR's core diagnosis is accurate — AnimationParser.parse only resolves theme-map keys via context.theme.animations.containsKey(className) and has no bracket-syntax handler, so animate-[wiggle_1s_ease-in-out_infinite] silently no-ops. Removing the section is the right call.


Blocker

Customizing Theme example uses the wrong key format (inline comment on animation.md:117).

The PR description says "Use the Customizing Theme section directly below, which is the correct extension path," but that section's code example uses 'wiggle' as the theme map key. The parser looks up the full class token (e.g. 'animate-wiggle'), not the suffix. Every default confirms the pattern:

// lib/src/theme/defaults/animations.dart
'animate-spin': WindAnimationType.spin,   // key includes the prefix

A user following the Customizing Theme section after this PR lands hits the same silent no-op. Needs a one-line fix to 'animate-wiggle' before merge.


Pre-existing issues (not introduced here, worth a follow-up)

animation_parser_test.dart missing WindParser.clearCache() in setUp.
The test file has no setUp at all. Per .claude/rules/tests.md this is mandatory for all parser tests — the cache persists between tests and produces false-positive passes. The five named tests in this file are at risk of cross-test pollution.

canParse / parse contract mismatch.
canParse('animate-[wiggle_1s_ease-in-out_infinite]') returns true (the token starts with animate-), but parse silently ignores it. Per .claude/rules/parsers.md: "Arbitrary values precede theme lookups." AnimationParser is the only parser in the pipeline that has no bracket-syntax path at all. Recommend filing a follow-up issue — even if the answer is "AnimationParser intentionally rejects bracket syntax," that decision should be explicit (an early return styles when the token contains [) and tested.

No regression test for the silent no-op.
The behavior motivating this PR — animate-[...] producing animationType == null — is not pinned by any test. If someone later wires up bracket-syntax support, no test will flag the behavior change.


What's correct

  • Both the ToC entry and the <a name="arbitrary-values"> anchor are removed together — no dangling anchor.
  • skills/wind-ui/SKILL.md does not reference animate-[...] bracket syntax, so no skill update is needed.
  • README.md does not need updating (doc fix, not a roster change).
  • CHANGELOG format matches the project convention; issue ref (#83) is present. (Minor inline suggestion to tighten the bullet to one sentence.)

@anilcancakir
Copy link
Copy Markdown
Collaborator

Closed: infra test artifact from claude-issue-agent dry-run. Reverting the agent workflow; will revisit later.

@anilcancakir anilcancakir deleted the claude/issue-83-26483895483 branch May 27, 2026 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: animation.md claims arbitrary animate-[...] syntax works, but AnimationParser does not implement it

2 participants