-
Notifications
You must be signed in to change notification settings - Fork 14
Refactor core modules + revamp all demo pages #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
0bfcbff
build: update eslint to ES2022
ianmcburnie f677b0a
refactor: prevent-scroll-keys
ianmcburnie 369af55
refactor: next-id
ianmcburnie 90acf30
test: increase coverage
ianmcburnie a40178f
refactor: typeahead
ianmcburnie 08f5b21
refactor: makeup-exit-emitter
ianmcburnie 1c6778e
refactor: makeup-focusables
ianmcburnie 401d0f5
refactor: makeup-key-emitter
ianmcburnie 193ebf3
refactor: makeup-expander
ianmcburnie 4a5d96c
refactor: makeup-keyboard-trap
ianmcburnie a5b08dc
docs: fixed svg examples in screenreader-trap
ianmcburnie 9424abb
refactor: screenreader-trap
ianmcburnie 3a20ab0
test(screenreader-trap): increase coverage
ianmcburnie f6231bd
docs(screenreader trap): revamp demo
ianmcburnie 9892670
docs(screenreader-trap): simplified examples
ianmcburnie 926849e
refactor: makeup-modal
ianmcburnie d12b55f
docs(modal): revamp demo
ianmcburnie 1f8382a
docs(modal): more demo revamp
ianmcburnie 730d460
refactor: makeup-navigation-emitter
ianmcburnie b0bd894
test(navigation-emitter): increase coverage
ianmcburnie 70cadc4
docs(navigation-emitter): revamped demo
ianmcburnie 65d6f0b
refactor: makeup-roving-tabindex
ianmcburnie 1bd1a39
test(roving-tabindex): increased coverage
ianmcburnie aa8c160
docs(roving-tabindex): revamped demo
ianmcburnie b31302c
refactor: makeup-active-descendant
ianmcburnie b91e92f
test(active-descendant): increase coverage
ianmcburnie 9575fb8
docs(active-descendant): revamped demo
ianmcburnie 3964428
docs(exit-emitter): revamp demo
ianmcburnie 44fe450
docs(expander): revamp demo
ianmcburnie 110c10b
docs(focusables): revamp demo
ianmcburnie e3aa7ba
docs(key-emitter): revamp demo
ianmcburnie 70ebd7c
docs(keyboard-trap): revamp demo
ianmcburnie 159c695
docs(next-id): revamp demo
ianmcburnie da5c2e8
docs(prevent-scroll): revamp demo
ianmcburnie c5f8aa2
docs(typeahead): revamp demo
ianmcburnie 30db807
docs: couple of minor readme updates
ianmcburnie 799536d
test(toast-dialog): fix broken selector
ianmcburnie 43d5fad
docs: ui cleanup
ianmcburnie 256dcba
feat: added claude skills
ianmcburnie 936c499
fix: remove broken nx daemon from start script, add dedicated playwri…
ianmcburnie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| --- | ||
| description: Write missing unit tests for a makeup-js module based on coverage gaps | ||
| args: `<module-name>` | ||
| --- | ||
|
|
||
| You are writing missing unit tests for a makeup-js module. Your goal is to fill coverage gaps with well-structured tests that follow the existing conventions — not to rewrite existing tests or pad coverage with low-value assertions. | ||
|
|
||
| **Input** | ||
|
|
||
| The user will provide a module name (e.g., "makeup-next-id" or full path like "packages/core/makeup-next-id"). Optionally, the user may paste a gap report from `/test-coverage` — if so, use it to skip re-running coverage and go straight to writing. | ||
|
|
||
| **Process** | ||
|
|
||
| **1. Locate the module** | ||
|
|
||
| - Find the module in `packages/core/` or `packages/ui/` | ||
| - Read `src/index.js` (or all files in `src/`) | ||
| - Read `test/index.js` (or all files in `test/`) | ||
|
|
||
| **2. Identify gaps** | ||
|
|
||
| If the user did not provide a gap report, run coverage to find them: | ||
|
|
||
| ``` | ||
| npm run test:coverage -- --coverage.include="packages/core/<module-name>/src/**" packages/core/<module-name> | ||
| ``` | ||
|
|
||
| Read the source and tests side by side to identify: | ||
|
|
||
| - Uncovered branches (`if`/`else`, ternaries, `?.`, `??`) | ||
| - Uncovered functions | ||
| - Missing edge cases (empty input, boundary values, error conditions) | ||
| - Missing negative cases (invalid input that is guarded in code) | ||
|
|
||
| Skip gaps that are not worth testing (non-deterministic internals, unreachable defensive guards, browser-only paths that jsdom cannot exercise). | ||
|
|
||
| **3. Write the tests** | ||
|
|
||
| Append the new tests to `test/index.js`. Do not modify existing tests. | ||
|
|
||
| Follow the existing conventions exactly: | ||
|
|
||
| - BDD structure: `describe("given X") > describe("when Y") > it("should Z")` | ||
| - One assertion per `it` block where practical | ||
| - Use `beforeEach`/`beforeAll` for setup, matching the pattern already in the file | ||
| - Use `vi.spyOn()` for mocking, `vi.useFakeTimers()` for timers — restore in `afterEach` | ||
| - Import only what is needed; do not add imports already present at the top of the file | ||
| - Match the indentation and formatting of the existing file | ||
|
|
||
| **4. Run and verify** | ||
|
|
||
| Run the tests to confirm they pass: | ||
|
|
||
| ``` | ||
| npm run test:unit -- packages/core/<module-name> | ||
| ``` | ||
|
|
||
| If any test fails, fix it before proceeding. Do not leave failing tests. | ||
|
|
||
| Then re-run coverage to confirm the gaps are closed: | ||
|
|
||
| ``` | ||
| npm run test:coverage -- --coverage.include="packages/core/<module-name>/src/**" packages/core/<module-name> | ||
| ``` | ||
|
|
||
| **5. Summary** | ||
|
|
||
| Report: | ||
|
|
||
| - Which gaps were addressed and which were intentionally skipped (and why) | ||
| - Updated coverage numbers | ||
| - Number of tests added |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| --- | ||
| description: Systematically refactor a makeup-js module with modern JavaScript patterns | ||
| args: <module-name> | ||
| --- | ||
|
|
||
| You are refactoring a module in the makeup-js monorepo to use modern JavaScript features while maintaining readability and test compatibility. | ||
|
|
||
| # Input | ||
|
|
||
| The user will provide a module name (e.g., "makeup-next-id" or full path like "packages/core/makeup-next-id"). | ||
|
|
||
| # Process | ||
|
|
||
| ## 1. Locate and analyze the module | ||
|
|
||
| - Find the module in `packages/core/` or `packages/ui/` | ||
| - Read the README.md **first** — it describes intended behavior from the consumer's perspective and is the authoritative source for what the module should do. Use it to inform all refactoring decisions, especially around edge cases and event behavior. | ||
| - Read the source file(s) in `src/` | ||
| - Read the test file(s) in `test/` | ||
| - Check the package.json for any specific configuration | ||
|
|
||
| ## 2. Analyze refactoring opportunities | ||
|
|
||
| Identify modernization opportunities based on browserslist (modern browsers): | ||
|
|
||
| **Prioritize readability over cleverness** | ||
|
|
||
| Common patterns to modernize: | ||
|
|
||
| - Replace deprecated `keyCode` with `key` property | ||
| - Replace magic numbers with named constants | ||
| - Use `const`/`let` consistently (no `var`) | ||
| - Replace `Object.assign({}, obj1, obj2)` with spread: `{...obj1, ...obj2}` — rename the result only if the existing name is genuinely unclear; do not rename variables solely to remove a `_` prefix | ||
| - Use arrow functions for callbacks and simple functions | ||
| - Use `Set` or `Map` where appropriate for better performance/readability | ||
| - Remove unnecessary `"use strict"` (not needed in ES modules) | ||
| - Use template literals for string concatenation | ||
| - Use optional chaining `?.` where it improves readability | ||
| - Use nullish coalescing `??` for default values (prefer over `||` when appropriate) | ||
| - Use `Array.from()`, `.forEach()`, `.map()`, `.filter()` etc. where readable | ||
| - Do NOT simplify `=== true` / `=== false` comparisons — they carry type information that TypeScript would otherwise provide. Leave them as-is. | ||
|
|
||
| **Avoid over-engineering:** | ||
|
|
||
| - Don't use private fields (`#`) unless there's a clear encapsulation benefit | ||
| - Don't restructure class architecture unnecessarily | ||
| - Keep logic simple and straightforward | ||
|
|
||
| **Preserve existing comments:** | ||
|
|
||
| - Do NOT remove comments from code you are refactoring | ||
| - Comments often explain non-obvious behavior, browser quirks, or intentional decisions that aren't evident from the code alone | ||
| - Only remove a comment if it is purely restating what the code already clearly says (e.g. `i++ // increment i`) | ||
|
|
||
| **Optimize for tree shaking:** | ||
|
|
||
| - Prefer named exports over default exports where possible | ||
| - Prefer named imports (`import { foo } from "pkg"`) over namespace imports (`import * as Pkg from "pkg"`) — named imports allow bundlers to tree-shake unused exports | ||
|
|
||
| **CRITICAL - No API changes:** | ||
|
|
||
| - Maintain existing API surface exactly | ||
| - Do NOT change exported functions, classes, or methods | ||
| - Do NOT change function signatures (parameters, return types) | ||
| - Do NOT change event names or event detail structures | ||
| - Do NOT introduce breaking changes | ||
| - This is internal refactoring only - the public API must remain unchanged | ||
|
|
||
| ## 3. Explain your understanding and ask questions | ||
|
|
||
| Before making any suggestions or changes, write a plain-text explanation of the module: | ||
|
|
||
| - What the module does from a consumer's perspective (what problem it solves, how it is used) | ||
| - What its key internal mechanisms are (how it works) | ||
| - Any non-obvious design decisions you noticed (e.g. why certain patterns are used, what tradeoffs are made) | ||
| - Any questions you have — about intent, edge cases, or anything in the source or tests that is unclear or surprising | ||
|
|
||
| Then **wait for the user to respond** before proceeding. Do not begin refactoring until the user has confirmed your understanding is correct and answered any questions. | ||
|
|
||
| ## 5. Refactor tests FIRST | ||
|
|
||
| **CRITICAL:** Maintain the given/when/then BDD test format. | ||
|
|
||
| Test improvements: | ||
|
|
||
| - Remove redundant import existence checks | ||
| - Replace `keyCode` with `key` in KeyboardEvent tests | ||
| - Use `vi.spyOn()` instead of manually mocking functions | ||
| - Add specific test cases for edge cases | ||
| - Use descriptive test names following given/when/then | ||
| - Ensure proper cleanup in beforeEach/afterEach | ||
| - Structure: `describe("given X") > describe("when Y") > it("should Z")` | ||
|
|
||
| ## 6. Refactor module code | ||
|
|
||
| Before writing any code changes, explain each planned source change as a learning opportunity: | ||
|
|
||
| - State what is changing and show the before/after | ||
| - Explain _why_ the new form is preferred — the underlying principle, not just the rule (e.g. what problem does `Object.assign` have that spread solves? why does eliminating the intermediate variable improve readability?) | ||
| - Connect the change to a broader pattern where relevant (e.g. "this is the same idea as destructuring — pulling out only what you need") | ||
|
|
||
| Then apply all changes. | ||
|
|
||
| ## 7. Update README | ||
|
|
||
| - Verify README is in sync with the refactored code | ||
| - Check that: | ||
| - Usage examples are accurate (no API changes, so should still work) | ||
| - Code examples use correct variable names and syntax | ||
| - Examples are clear and follow current code | ||
| - **Verify every listed property actually exists** — check each entry in the Properties section against the source. Look for getters, public instance fields, and properties set in the constructor. Remove or correct any that don't exist. | ||
| - **Verify every listed method actually exists** — check each entry in the Methods section against the source. Look for class methods and exported functions. | ||
| - **Verify every listed event name matches a `dispatchEvent` call** — search the source for `new CustomEvent(...)` and confirm the names match what the Events section documents. | ||
| - **Verify every listed option is present in `defaultOptions`** — confirm each option key in the Options section exists in the module's default options object. | ||
| - Fix any typos or inconsistencies in examples | ||
| - Do NOT change the API in the README - this is verification only | ||
| - Remove the "Dependencies" section if present — it duplicates package.json and drifts out of sync | ||
| - Keep documentation concise and accurate | ||
|
|
||
| ## 8. Compile and test | ||
|
|
||
| - Run `npm run compile --workspace=<module-name>` | ||
| - Run `npm run test:unit -- packages/core/<module-name>` or `packages/ui/<module-name>` | ||
| - Verify all tests pass | ||
| - If tests fail, analyze and fix | ||
|
|
||
| ## 9. Summary | ||
|
|
||
| Provide a concise summary: | ||
|
|
||
| - Module name and location | ||
| - Test improvements made | ||
| - Code improvements made | ||
| - Test results | ||
| - Lines of code before/after (if significantly different) | ||
|
|
||
| # Browser Support | ||
|
|
||
| Target modern browsers per @ebay/browserslist-config. All modern ES features are supported. | ||
|
|
||
| # Testing | ||
|
|
||
| Tests use Vitest. Maintain compatibility with existing test infrastructure. | ||
|
|
||
| # Output Format | ||
|
|
||
| Be concise. Show what was changed and why. Confirm tests pass. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.