Conversation
|
There was a problem hiding this comment.
Pull request overview
Updates the evo-migrate-marko Claude skill guidance based on an audit, aiming to improve how the skill is selected and to provide more precise migration instructions for specific accessibility/focus-management patterns.
Changes:
- Expands skill description to encourage using the skill when users discuss migrating/porting ebay-* Marko components.
- Refines guidance around
makeup-*library replacements (roving tabindex vs active-descendant patterns) and adds a note aboutmakeup-focusables. - Adds a checklist of expected
README.mdcontents for migrated components.
|
Ignoring all Copilot suggestions from this PR as it doesn't have enough Claude context and is approaching things from a human readability angle rather than skill efficiency. |
| - `makeup-expander` + `@floating-ui/dom` -> `<evo-expander>` (in `src/tags/tags/`) | ||
| - `makeup-floating-label` -> `<evo-floating-label>` (in `src/tags/tags/`) | ||
| - `makeup-typeahead` -> `<evo-typeahead>` (in `src/tags/tags/`) | ||
| - `makeup-focusables` -> **no evo replacement**; use native `querySelectorAll` with a standard focusable selector instead |
There was a problem hiding this comment.
Not necessarily true, we may end up making a Marko equivalent of this
There was a problem hiding this comment.
@LuLaValva , the future is not very relevant with respect to the skill. It's what is true right now. Since this is an ephemeral skill anyway, the future may be wholesale irrelevant. As the codebase exists now, is the statement true for the skill to get enough context to do a better job. This is the only question we should be asking.
There was a problem hiding this comment.
I understand that, but this isn't necessarily the solution we want to use for migrating makeup-focusables and I think it is a hallucination
There was a problem hiding this comment.
I'm not sure whether it's a hallucination or not, but it is trying to make explicit what is currently implied. What's merely implied creates vagueness and leads to non-deterministic outcomes. That said, your concern is fair. What is the right solution? Maybe we can capture that explicitly.
There was a problem hiding this comment.
We don't know what the right solution is yet because we haven't migrated the combobox component yet. I will add the correct solution when I migrate a component that uses this pattern and have had time to figure out what we should be doing in tags API
| - **Basic usage example** (copy-pasteable Marko snippet) | ||
| - **Props table** — name, type, default, description (derive from the `Input` interface) | ||
| - **Accessibility notes** — any keyboard interactions, ARIA roles/attributes, screen reader behavior | ||
| - **Breaking changes from ebayui-core** — renamed props, removed props, changed event signatures |
There was a problem hiding this comment.
We shouldn't include breaking changes from eBayUI in the README. We'll figure out a better migration path once I've moved all components over
There was a problem hiding this comment.
This is not the README, it's the SKILL. If the statements are true, we should keep them in since their purpose is to help the skill do a better job of migrating components. If they need adjustment because of some nuance, we should adjust them. If they are wholesale inaccurate, then we should remove/exclude them.
There was a problem hiding this comment.
This is a part of the skill that is instructing AI how to migrate a README from ebayui-core to evo-marko. The statements are not true, as these changes should not be made to the README
There was a problem hiding this comment.
Gotcha. Ok. Can you suggest alternative wording of what should go in the README?
There was a problem hiding this comment.
I think we should either remove this section or tell it to copy the README directly and find & replace ebay-* to evo-*
|
|
||
| - **One-line description** of what the component does | ||
| - **Basic usage example** (copy-pasteable Marko snippet) | ||
| - **Props table** — name, type, default, description (derive from the `Input` interface) |
There was a problem hiding this comment.
We should NOT have a props table in the README, it will drift out of sync very quickly. There is no reason to add another maintenance point
There was a problem hiding this comment.
This is the SKILL, not the README. I think this is meant to provide a template/pattern. Excluding them will leave it up the model's understanding and will make this non-deterministic.
The size of the skill is irrelevant. We're not measuring the tokens in the skill. The performance evals are based on the efficiency and effectiveness of the |
|
Closing this so Luke can make the changes himself. |
Description
I put the current
evo-migrate-markoskill through an audit using theskill-creatorskill and used the conservative set of changes it suggested to slightly improve the skill.Notes
There was a broader set of suggested changes to the skill that did not dramatically improve the end result of the skill. However, it did reduce the token usage and took less time to run. The broader audit also included evals for baselines on performance and efficiency of the skill. I left that out of this PR, too. If desired, that can be done as a separate PR by running
skill-creatoraudit on the skill at any time.This is the benchmark dashboard comparing the efficiency of the current skill with the potential efficiency gains from the revised skill (with all possible improvements).