fix(bricks): natural-sort palette so -50 lands before -500 + reBEMer design refinements - #129
Merged
Merged
Conversation
The Bricks color palette dropdown was rendering shades in lexicographic
order, so users saw primary-100, primary-200, ..., primary-50, primary-500,
primary-600 — with -50 stranded between -400 and -500 instead of right
after the base color. Same issue affected alpha shades (a5 after a40)
and any other numeric-suffix token (space-2 after space-10 etc.).
Switch every CSS-name sort to SORT_NATURAL | SORT_FLAG_CASE so numeric
suffixes order intuitively. Five sites in three files:
- includes/class-css-parser.php (variables, class names)
- includes/class-inventory.php (sanitize_inventory, two in
get_variables_by_category)
- scripts/gen-bricks-inventory.js (generator, regenerated output)
The PHP sort is what feeds Bricks' Color Manager, so this is the only
required change for the user-visible bug. The JS generator is updated
in lockstep so the committed fallback inventory.json reads in the same
order — a cosmetic improvement (sanitize_inventory always re-sorts on
read).
The token-api snapshot test is unaffected: it does its own JS-side
[...names].sort() of declared tokens and compares against the locked
snapshot. Lex-sort there is fine — that contract is about the *set*
of tokens, not the order.
Also extends docs/rebemer.md with the v1 design refinements approved
in design review:
- Goal #9: element-aware row pre-fills, sibling auto-numbering,
per-row skip toggle (without breaking apply atomicity)
- §6.2: skip-toggle column documented in the panel sketch
- §6.3: migrate-mode preview chip strip
- §7.1/7.2: new lib/element-types.js module + tests
- §9: Operation gains skip and suggestedFrom fields
- §9.2: sibling auto-numbering rule (plan-local, excludes skipped)
- §9.3: element-type pre-fill + provenance tracking
- §11.3: nameCollisions entries gain recommendedAction hint
(attach / rename / replace) for one-click 'use existing'
- §20: unused-class read-only report added to v1.1 candidates
Co-authored-by: Jack Granatowski <contact@codeslash.net>
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces reBEMer v1 design refinements covering skip/include UI semantics and element-aware naming, plus standardizes Bricks inventory token and class ordering to natural-case sorting across the generation pipeline and output JSON. ChangesreBEMer v1 Design Document
Natural-case Sorting Standardization for Bricks Inventory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/gen-bricks-inventory.js (1)
49-49: ⚡ Quick winPin the comparator locale/collator for deterministic generated inventory ordering.
localeComparewith anundefinedlocale relies on the runtime/host default locale, which can vary across environments and lead to diff churn; use a single explicitIntl.Collatorand reuse its.compare.Proposed diff
-const naturalCompare = (a, b) => a.localeCompare(b, undefined, { numeric: true, sensitivity: 'base' }); +const naturalCompare = new Intl.Collator('en', { + numeric: true, + sensitivity: 'base', +}).compare;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gen-bricks-inventory.js` at line 49, The comparator naturalCompare currently calls a.localeCompare with an undefined locale which makes ordering platform-dependent; replace it by instantiating a single explicit Intl.Collator (e.g., new Intl.Collator('en', { numeric: true, sensitivity: 'base' })) and reuse its .compare method for naturalCompare so the generated inventory ordering is deterministic; ensure the collator is created once (module scope) and that naturalCompare references collator.compare.docs/rebemer.md (1)
176-176: ⚡ Quick winSimplify redundant phrasing.
The phrase "lifted up into" is redundant. Consider "lifted into" or "moved into" for clearer, more concise documentation.
📝 Suggested simplification
-chip strip listing the element-settings keys (and a hover tooltip with -their values) that will be lifted up into the new class. Drawn from +chip strip listing the element-settings keys (and a hover tooltip with +their values) that will be lifted into the new class. Drawn from🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rebemer.md` at line 176, Replace the redundant phrase "lifted up into" (found in the docs line containing "their values) that will be lifted up into the new class. Drawn from") with a concise alternative such as "lifted into" or "moved into"; update the sentence to read e.g. "that will be moved into the new class" and ensure surrounding punctuation and capitalization remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rebemer.md`:
- Around line 179-180: The phrase "on the allowlist denylist" in docs/rebemer.md
(near the §12 reference "Bricks introducing a new style key") is ambiguous;
update that sentence to use a single clear term — replace "on the allowlist
denylist" with "on the denylist" (or "not on the allowlist" if the intent is
absence from the allowlist) so it unambiguously conveys blocked settings; find
the sentence containing that phrase and make the replacement to match the
surrounding discussion of surfacing blocked settings.
---
Nitpick comments:
In `@docs/rebemer.md`:
- Line 176: Replace the redundant phrase "lifted up into" (found in the docs
line containing "their values) that will be lifted up into the new class. Drawn
from") with a concise alternative such as "lifted into" or "moved into"; update
the sentence to read e.g. "that will be moved into the new class" and ensure
surrounding punctuation and capitalization remain correct.
In `@scripts/gen-bricks-inventory.js`:
- Line 49: The comparator naturalCompare currently calls a.localeCompare with an
undefined locale which makes ordering platform-dependent; replace it by
instantiating a single explicit Intl.Collator (e.g., new Intl.Collator('en', {
numeric: true, sensitivity: 'base' })) and reuse its .compare method for
naturalCompare so the generated inventory ordering is deterministic; ensure the
collator is created once (module scope) and that naturalCompare references
collator.compare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed6a7dde-57cd-45b3-b67c-072dc90ed427
📒 Files selected for processing (5)
docs/rebemer.mdintegrations/bricks/data/inventory.jsonintegrations/bricks/includes/class-css-parser.phpintegrations/bricks/includes/class-inventory.phpscripts/gen-bricks-inventory.js
Comment on lines
+179
to
+180
| when a setting *would* be migrated but is on the allowlist denylist | ||
| (see §12 threat-model entry "Bricks introducing a new style key"). |
There was a problem hiding this comment.
Clarify "allowlist denylist" phrasing.
The phrase "on the allowlist denylist" is confusing — a key cannot be on both simultaneously. Based on the context (surfacing blocked settings), this should likely read "on the denylist" or "not on the allowlist."
📝 Suggested fix
-when a setting *would* be migrated but is on the allowlist denylist
+when a setting *would* be migrated but is on the denylistOr, if the intent is to describe absence from the allowlist:
-when a setting *would* be migrated but is on the allowlist denylist
+when a setting *would* be migrated but is not on the allowlist📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| when a setting *would* be migrated but is on the allowlist denylist | |
| (see §12 threat-model entry "Bricks introducing a new style key"). | |
| when a setting *would* be migrated but is on the denylist | |
| (see §12 threat-model entry "Bricks introducing a new style key"). |
Suggested change
| when a setting *would* be migrated but is on the allowlist denylist | |
| (see §12 threat-model entry "Bricks introducing a new style key"). | |
| when a setting *would* be migrated but is not on the allowlist | |
| (see §12 threat-model entry "Bricks introducing a new style key"). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rebemer.md` around lines 179 - 180, The phrase "on the allowlist
denylist" in docs/rebemer.md (near the §12 reference "Bricks introducing a new
style key") is ambiguous; update that sentence to use a single clear term —
replace "on the allowlist denylist" with "on the denylist" (or "not on the
allowlist" if the intent is absence from the allowlist) so it unambiguously
conveys blocked settings; find the sentence containing that phrase and make the
replacement to match the surrounding discussion of surfacing blocked settings.
Contributor
|
/kiro all |
CodeRabbit correctly flagged the contradictory 'on the allowlist denylist' wording — a key can't be on both simultaneously. The intent is to surface settings that are NOT on the allowlist (i.e. unknown keys that reBEMer refuses to migrate). Updated to 'not on the allowlist' for clarity.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request was created by @kiro-agent on behalf of @jackgranatowski 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro autonomous agent
What
Two related changes from this design-review pass:
1. Bug fix: Bricks color palette ordering
Shades were rendering in lexicographic order, so
primary-50landed betweenprimary-400andprimary-500instead of right after the base color. Same issue affected alpha shades (a5aftera40) and any other numeric-suffix token (space-2afterspace-10, etc.).Fix: every CSS-name sort now uses
SORT_NATURAL | SORT_FLAG_CASEso numeric suffixes order intuitively.Five sites in three files:
integrations/bricks/includes/class-css-parser.php(variables, class names)integrations/bricks/includes/class-inventory.php(sanitize_inventory, two inget_variables_by_category)scripts/gen-bricks-inventory.js(generator + regenerated output for a clean diff)The PHP sort is what feeds Bricks' Color Manager, so that alone fixes the user-visible bug. The JS generator is updated in lockstep so the committed fallback
inventory.jsonreads in the same order — purely cosmetic, sincesanitize_inventory()always re-sorts on read.Before / After (primary shades):
2. Design refinements added to
docs/rebemer.mdv1 design extensions approved in the design-review thread (Auto-BEM parity gaps + one v1.1 candidate):
lib/element-types.jsmodule + testsOperationgainsskipandsuggestedFromfieldsnameCollisionsentries gainrecommendedActionhint (attach/rename/replace) for a one-click "use existing" affordanceTested
php -lclean on both modified PHP filestests/token-api.spec.jsstill green (independent JS-side sort, snapshot unaffected)dist/slashed.optimal.css— primary shades emerge inprimary, -50, -100, …, -500, -600, …, -950, -a5, -a10, -a20, …orderinventory.jsonregenerated and diffed: only the expected shade reorderings, no other tokens movednpx playwright install); they test computed CSS values, not name orderingOut of scope
tests/token-api.spec.jsand its snapshot were intentionally not changed. That contract is about the set of tokens, not the order — switching it to natural sort would force a noisy snapshot churn for no behavior change.Summary by CodeRabbit
Documentation
Chores