feat(blade-svelte): proposal evaluation#3481
Draft
rohankokane-dev wants to merge 1 commit into
Draft
Conversation
Land working implementations for all five styling options (A–E) on Button and Card so the team can compare them in Storybook before adopting Blade in Checkout Config V2. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Contributor
|
✅ PR title follows Conventional Commits specification. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 98e68c4:
|
Collaborator
🛡️ Coverage ReportSummaryFull Coverage Details |
![rzp-slash-public[bot]](https://avatars.githubusercontent.com/u/7713209?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
AI Code Review
Found 4 inline suggestion(s).
Related Suggestions
These files are not part of this PR but may need attention:
.changeset/:0 [PRE_MORTEM, importance: 6]
The PR adds new exported APIs to two published packages —
@razorpay/blade-core(resolveButtonOverrides, resolveCardOverrides, styleObjectToString, ButtonStyleOverrides, CardStyleOverrides, SlotThemeMap, ThemeOverrideTree, flattenThemeOverridesToVars, …) and@razorpay/blade-svelte(BladeProvider, styleOverrides prop on Button/Card) — but the diff contains no new.changeset/*.mdfile. The PR checklist also marks "Add changeset" as unchecked. Without a changeset entry, Changesets tooling will not bump the package version or generate release notes, and the new public API ships with no version bump.
Tools Used
Skills: pre-mortem, review
| // - Option A visual inline styles (background-color/color/border written directly) | ||
| // - Option B/D override CSS variables (state-aware, the recommended carrier) | ||
| // Override vars are emitted last so they take precedence over A on the same concern. | ||
| const combinedInlineStyle = $derived( |
Contributor
There was a problem hiding this comment.
Suggestion: styleObjectToString emits camelCase CSS props (backgroundColor, marginLeft) — invalid in HTML style string; Option A visualProps silently fails [SVELTE, importance: 7]
Source: sub-agent: svelte | skill: review
💡 View suggested fix (click to expand)
Suggested change
| const combinedInlineStyle = $derived( | |
| // styleObjectToString does a literal `${key}:${value}` join. CSS custom | |
| // properties (--btn-*) are already kebab-case and work fine. But regular CSS | |
| // properties returned by resolveVisualStyledProps (backgroundColor, borderColor, | |
| // borderRadius) and getStyledPropsClasses (marginLeft, zIndex, gridColumn…) are | |
| // camelCase — browsers silently ignore unknown property names in style attributes. | |
| // Svelte converts camelCase→kebab-case only when you use style={{object}}, not | |
| // for string style attributes. | |
| const combinedInlineStyle = $derived( | |
| Object.entries({ | |
| ...styledProps.inlineStyles, | |
| ...visualInlineStyles, | |
| ...overrideVars, | |
| }) | |
| .filter(([, v]) => v !== undefined && v !== '') | |
| .map(([k, v]) => { | |
| // CSS custom properties start with '--' and must NOT be converted. | |
| const prop = k.startsWith('--') ? k : k.replace(/([A-Z])/g, '-$1').toLowerCase(); | |
| return `${prop}:${v}`; | |
| }) | |
| .join(';'), | |
| ); |
| @@ -0,0 +1,100 @@ | |||
| import tinycolor from 'tinycolor2'; | |||
Contributor
There was a problem hiding this comment.
Suggestion: Six new files with non-trivial business logic ship without any co-located .test.ts files. The repo pattern (lodashButBetter/*.test.ts, overrideTheme.test.ts) is to co-locate tests alongside each utility. The HIGHLIGHTED_DARKEN_AMOUNT (8), DISABLED_ALPHA (0.5), and the darken/fade math in deriveColorStates are the centerpiece of v1 (per the proposal) — an incorrect coefficient silently ships broken hover/disabled states for every consumer. Also missing tests for: visualStyledProps.ts, buttonOverrides.ts, cardOverrides.ts, slotTheme.ts, themeScope.ts.
[PRE_MORTEM, importance: 7]
Source: sub-agent: pre_mortem | skill: pre-mortem
💡 View suggested fix (click to expand)
Suggested change
| import tinycolor from 'tinycolor2'; | |
| // packages/blade-core/src/utils/colorOverrides/deriveColorStates.test.ts | |
| import { deriveColorStates, darkenColor, fadeColor, HIGHLIGHTED_DARKEN_AMOUNT, DISABLED_ALPHA } from './deriveColorStates'; | |
| describe('deriveColorStates', () => { | |
| it('returns default/highlighted/disabled triplet from a hex', () => { | |
| const result = deriveColorStates('#1a59ff'); | |
| expect(result.default).toBe('#1a59ff'); | |
| expect(result.highlighted).not.toBe('#1a59ff'); // darkened | |
| expect(result.disabled).toMatch(/^rgba/); // alpha applied | |
| }); | |
| it('darkenColor falls back to original string for invalid color', () => { | |
| expect(darkenColor('not-a-color')).toBe('not-a-color'); | |
| }); | |
| it('fadeColor falls back to original string for invalid color', () => { | |
| expect(fadeColor('not-a-color')).toBe('not-a-color'); | |
| }); | |
| it('isOverrideColorValue rejects empty string and non-strings', () => { | |
| expect(isOverrideColorValue('')).toBe(false); | |
| expect(isOverrideColorValue(null)).toBe(false); | |
| expect(isOverrideColorValue('#ff0000')).toBe(true); | |
| }); | |
| }); | |
|
|
||
| // ===== Instance-level styling (Options A–E) ===== | ||
| // Option D: resolve this button's slot from the nearest BladeProvider slotTheme. | ||
| const slotThemeMap = getSlotThemeMap(); |
Contributor
There was a problem hiding this comment.
Suggestion: Context getter resolved outside $derived — slotTheme prop changes in BladeProvider won't reactively re-evaluate slotOverrideVars [SVELTE, importance: 7]
Source: sub-agent: svelte | skill: review
💡 View suggested fix (click to expand)
Suggested change
| const slotThemeMap = getSlotThemeMap(); | |
| // getSlotThemeMap() resolves the getter immediately and returns a plain value — | |
| // since slotThemeMap is not a reactive signal, $derived never re-runs when the | |
| // BladeProvider's slotTheme prop changes. | |
| // | |
| // Fix: expose the raw getter from bladeProviderContext (getSlotThemeContext) so | |
| // the getter is called *inside* $derived, making Svelte track slotTheme reactively. | |
| // | |
| // In bladeProviderContext.ts add: | |
| // export function getSlotThemeContext() { | |
| // return getContext<(() => SlotThemeMap | undefined) | undefined>(SLOT_THEME_CONTEXT_KEY); | |
| // } | |
| // | |
| // Then replace lines 69-70 here with: | |
| const slotThemeContext = getSlotThemeContext(); // gets the getter fn at init time (getContext call) | |
| const slotOverrideVars = $derived(resolveSlotTheme(slotThemeContext?.(), 'button', themeKey)); | |
| * a `backgroundColor` here dead-ends hover/disabled. Prefer `styleOverrides`. | ||
| */ | ||
| visualProps?: VisualStyledProps; | ||
| /** |
Contributor
There was a problem hiding this comment.
Suggestion: Three props explicitly labelled "evaluation only" — visualProps (Option A), themeKey (Option D), and className (Option E) — are added to the public BaseButtonProps interface and exported to consumers. The proposal's own verdict rejects Options A/D/E. Once these land in a released package they become a de-facto contract: removing them is a breaking change. Consumers scanning JSDoc see visualProps export from blade-core/utils (VisualStyledProps, resolveVisualStyledProps).
[PRE_MORTEM, importance: 6]
Source: sub-agent: pre_mortem | skill: pre-mortem
💡 View suggested fix (click to expand)
Suggested change
| /** | |
| // Option 1 — Omit evaluation props from the shipped interface entirely. | |
| // Keep them in a separate internal type used only by the stories. | |
| // Option 2 — Mark as @internal so typedoc / IDE tooling hides them: | |
| /** | |
| * @internal Evaluation-only. Will be removed before stable release. | |
| * @deprecated Use styleOverrides (Option B) instead. | |
| */ | |
| visualProps?: VisualStyledProps; | |
| // Option 3 — Gate behind a build flag so they tree-shake out of production bundles. | |
Contributor
PR Risk Assessment: 🟡 MEDIUMRule Source: standard_rules ReasoningThis PR adds a new instance-level styling API (Options A–E) to 🔍 Pre-Mortem AnalysisCategories Checked:
Issues by Severity:
Files Affected:
Analysis Summary:
Sub-Agent Attribution
Assessed by Slash Reviewer |
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.
Land working implementations for all five styling options (A–E) on Button and Card so the team can compare them in Storybook before adopting Blade in Checkout Config V2.
Description
Changes
Additional Information
Component Checklist