[DG-243] Fix button schema padding mismatch with CWButton defaults#476
[DG-243] Fix button schema padding mismatch with CWButton defaults#476carbonFibreCode wants to merge 3 commits into
Conversation
WalkthroughCompute button disabled state from props/onClick; derive per-state background/text/leading/trailing icon colors; extend ButtonStyle with fixedSize/minimumSize/tapTargetSize/visualDensity; update onPressed to respect disabled and call onClick; refactor content builder to accept explicit color overrides and conditionally render icons. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 3
🧹 Nitpick comments (1)
lib/src/framework/widgets/button.dart (1)
91-93: Consider simplifying the isDisabled logic.The condition checks
buttonState == ButtonState.disabledStatein addition toisDisabledandonClick == null. Since the button's visual styling is already controlled bybuttonState, this additional check may be redundant for interactivity purposes.If the intention is to disable interaction whenever
buttonStateis explicitly set to disabled, consider whether this should override theisDisabledprop or if the logic can be simplified:final isDisabled = payload.eval<bool>(props.get('isDisabled')) ?? - props.get('onClick') == null || - buttonState == ButtonState.disabledState; + props.get('onClick') == null;Alternatively, document the interaction between
isDisabled,onClick, andbuttonStatefor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/framework/widgets/button.dart(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-format-and-analyse
🔇 Additional comments (4)
lib/src/framework/widgets/button.dart (4)
20-23: LGTM!The ButtonState enum is well-defined and provides clear state management for the button.
184-193: LGTM!The
_getButtonStatehelper method correctly maps string values to the ButtonState enum with appropriate defaults.
66-69: Good fix for padding mismatch.The addition of
tapTargetSize: MaterialTapTargetSize.shrinkWrap,visualDensity: VisualDensity.standard, andminimumSize: Size.zerocorrectly addresses the PR objective of matching CWButton defaults. These settings remove Flutter's default padding and allow precise control over button sizing.
143-143: No issues found - null handling is correct.The
ExprOr.fromJson<T>()method properly handles null values. It acceptsObject?as a parameter and includes an explicit null check at line 83 of types.dart that returnsnullgracefully. BothleadingIconColorandtrailingIconColorare nullableString?values that may be passed to this method, and the method's return typeExprOr<T>?correctly handles both cases (non-null wrapped value or null).
| final backgroundColor = buttonState == ButtonState.defaultState | ||
| ? payload.evalColor(defaultStyleJson.getString('backgroundColor')) | ||
| : (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ?? | ||
| const Color(0xFFE0E0E0)); // AppColorsV2.contentDisabled fallback |
There was a problem hiding this comment.
Add null fallback for default state backgroundColor.
The backgroundColor only has a fallback (Color(0xFFE0E0E0)) for the disabled state. If defaultStyleJson.getString('backgroundColor') returns null for the default state, this could result in a null backgroundColor being passed to the button style, potentially causing unexpected rendering behavior.
Consider adding a fallback for the default state as well:
final backgroundColor = buttonState == ButtonState.defaultState
- ? payload.evalColor(defaultStyleJson.getString('backgroundColor'))
+ ? (payload.evalColor(defaultStyleJson.getString('backgroundColor')) ??
+ Colors.blue) // or appropriate default color
: (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ??
const Color(0xFFE0E0E0));📝 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.
| final backgroundColor = buttonState == ButtonState.defaultState | |
| ? payload.evalColor(defaultStyleJson.getString('backgroundColor')) | |
| : (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ?? | |
| const Color(0xFFE0E0E0)); // AppColorsV2.contentDisabled fallback | |
| final backgroundColor = buttonState == ButtonState.defaultState | |
| ? (payload.evalColor(defaultStyleJson.getString('backgroundColor')) ?? | |
| Colors.blue) // or appropriate default color | |
| : (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ?? | |
| const Color(0xFFE0E0E0)); // AppColorsV2.contentDisabled fallback |
🤖 Prompt for AI Agents
In lib/src/framework/widgets/button.dart around lines 41 to 44, the
default-state backgroundColor uses
payload.evalColor(defaultStyleJson.getString('backgroundColor')) without a null
fallback, so if getString returns null the backgroundColor may be null; update
the ternary branch to provide a fallback (e.g. wrap the evalColor(...) with a
null-coalescing fallback like ?? const Color(0xFFE0E0E0) or
AppColorsV2.contentDisabled) so both default and disabled branches always
produce a non-null Color.
| fixedSize: width != null || height != null | ||
| ? WidgetStateProperty.all( | ||
| Size(width ?? double.infinity, height ?? double.infinity)) | ||
| ? Size(width ?? double.infinity, height ?? double.infinity) | ||
| : null, |
There was a problem hiding this comment.
Fix Size construction to avoid infinite dimensions.
Using double.infinity as a fallback for width or height can cause layout issues. If only one dimension is provided, the other should not default to infinity, as this will force the button to expand infinitely in that dimension.
Apply this diff to fix the issue:
- fixedSize: width != null || height != null
- ? Size(width ?? double.infinity, height ?? double.infinity)
- : null,
+ fixedSize: width != null && height != null
+ ? Size(width, height)
+ : null,Alternatively, if you want to support setting only width or height individually, consider using minimumSize or maximumSize constraints instead of fixedSize.
🤖 Prompt for AI Agents
In lib/src/framework/widgets/button.dart around lines 74 to 76, the code
constructs a Size using double.infinity for the missing dimension causing
infinite layout; change the logic so fixedSize is only set when both width and
height are provided (use Size(width, height) then), and if only one dimension is
supplied do NOT use double.infinity — instead leave fixedSize null and apply a
minimumSize/maximumSize (or another appropriate constraint) for the
single-dimension case so the button does not expand infinitely.
| final JsonLike localProps = | ||
| jsonDecode(jsonEncode(props.value)) as JsonLike? ?? {}; |
There was a problem hiding this comment.
Performance concern: Avoid expensive JSON serialization for props cloning.
Using jsonDecode(jsonEncode(props.value)) for deep cloning is computationally expensive and will be executed on every render. This could impact performance, especially when many buttons are rendered in lists or grids.
Consider these alternatives:
- Create a dedicated method to clone the specific properties you need to modify
- Use a more efficient deep copy mechanism
- Build a new Props object with only the overridden values
Example approach:
// Instead of deep cloning entire props, create a modified version selectively
final textProps = as$<JsonLike>(props.value['text']).maybe(TextProps.fromJson) ?? TextProps();
final modifiedTextProps = textColor != null
? textProps.copyWith(textStyle: textProps.textStyle?.copyWith(color: textColor))
: textProps;
text = VWText(
props: modifiedTextProps,
commonProps: null,
parent: this,
).toWidget(payload);🤖 Prompt for AI Agents
In lib/src/framework/widgets/button.dart around lines 121-122, the code uses
jsonDecode(jsonEncode(props.value)) to deep-clone props which is expensive on
every render; replace this expensive full JSON round-trip by constructing a new
props object with only the fields you need to change (or implement a
light-weight clone/merge helper) — extract the specific child props (e.g., text
props), apply copyWith or manual field overrides to create a modified props
instance, and pass that to the child widget so you avoid serializing the entire
props map on each render.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/src/framework/widgets/button.dart (2)
37-40: ButtonStyle sizing & color fallbacks still have edge cases (null background + infinite dimensions)There are a few things worth tightening up in the style construction:
Enabled backgroundColor may still be null (duplicate of earlier review)
The disabled path has a safe fallback, but the enabled path does not. IfbackgroundColoris omitted fromdefaultStyle,WidgetStateProperty.all(backgroundColor)may receive null and yield unexpected visual behavior.
- final backgroundColor = isDisabled
? (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ??const Color(0xFFE0E0E0)): payload.evalColor(defaultStyleJson.getString('backgroundColor'));
- final backgroundColor = isDisabled
? (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ??const Color(0xFFE0E0E0)): (payload.evalColor(defaultStyleJson.getString('backgroundColor')) ??const Color(0xFFE0E0E0)); // or a design‑system primaryAdjust the fallback color to whatever your design system expects.
fixedSizewithdouble.infinitycan cause layout issues (duplicate of earlier review)
UsingSize(width ?? double.infinity, height ?? double.infinity)forces the unconstrained dimension to be infinite, which can blow up layouts in many flex / scroll contexts.A safer pattern is to only set
fixedSizewhen both dimensions are provided, and otherwise rely onminimumSize/ intrinsic sizing:
fixedSize: (width != null || height != null)? WidgetStateProperty.all(Size(width ?? double.infinity, height ?? double.infinity),): null,
fixedSize: (width != null && height != null)? WidgetStateProperty.all(Size(width, height)): null,
minimumSize: Size.zero+tapTargetSize: shrinkWrapreduces touch targets a lot
This combination effectively removes Material’s default 48×48 tap target. If that’s intentional and matches the product’s a11y guidelines, fine; otherwise you may want to keep the Material defaults or apply a smaller but non‑zero minimum to avoid hard‑to‑tap buttons, especially on mobile.For example, if you only want to shrink horizontally, you can keep a vertical minimum:
minimumSize: WidgetStateProperty.all(Size.zero),tapTargetSize: MaterialTapTargetSize.shrinkWrap,
minimumSize: WidgetStateProperty.all(const Size(0, 40)), // exampletapTargetSize: MaterialTapTargetSize.shrinkWrap,Also applies to: 59-60, 61-67, 72-73, 75-83
111-112: AvoidjsonEncode/jsonDecodedeep clone inside_buildContent
final JsonLike localProps = jsonDecode(jsonEncode(props.value)) as JsonLike? ?? {};does a full JSON round‑trip on every render, which is relatively expensive for a frequently built widget like a button, especially in lists or grids.Since
_buildContentonly needs to override a few specific sub‑trees (text props and leading/trailing icon props), you can avoid cloning the entire props map and instead construct just the parts you need:final textJson = as$<JsonLike>(props.value['text']); final textProps = textJson.maybe(TextProps.fromJson) ?? TextProps(); final effectiveTextProps = textColor != null ? textProps.copyWith( textStyle: textProps.textStyle?.copyWith(color: textColor), ) : textProps; // Similarly for leading/trailing icons using IconProps.fromJson + copyWithThen pass these typed props directly into
VWText/VWIconwithout an intermediary JSON clone. This should noticeably reduce per-build overhead.
🧹 Nitpick comments (2)
lib/src/framework/widgets/button.dart (2)
34-36: ClarifyisDisabledvsonClickinteraction to avoid null action execution
isDisableddefaults toprops.get('onClick') == null, but if a schema setsisDisabledtofalseexplicitly while leavingonClicknull,onPressedbecomes non-null andActionFlow.fromJson(props.get('onClick'))will receive a null payload, which may blow up at runtime depending onActionFlow.fromJson’s implementation.Consider tightening this coupling so the button never attempts to execute a null action, e.g. by deriving
isDisabledfrom both flags and reusing the sameonClickJSON you pass toActionFlow:- final isDisabled = payload.eval<bool>(props.get('isDisabled')) ?? - props.get('onClick') == null; + final onClickJson = props.get('onClick'); + final isDisabled = + payload.eval<bool>(props.get('isDisabled')) ?? onClickJson == null; ... - onPressed: isDisabled - ? null - : () { - final onClick = ActionFlow.fromJson(props.get('onClick')); + onPressed: isDisabled + ? null + : () { + if (onClickJson == null) return; + final onClick = ActionFlow.fromJson(onClickJson); payload.executeAction(onClick, triggerType: 'onPressed'); },This keeps the override behavior while guaranteeing the callback never sees a null JSON.
Also applies to: 93-99
42-52: Color override wiring looks good; tighten icon null-handling for consistencyThe new flow for propagating per‑state colors down into text and icons is a nice cleanup:
textColor,leadingIconColor, andtrailingIconColorare derived once fromdefaultStyleJson/disabledStyleJson._buildContentappliestextColorviasetValueFor('text.textStyle.textColor', ...)only when non‑null, preserving existing text styles otherwise.- Icons are conditionally rendered based on
*.iconDatapresence, and the Row always contains three children, which keeps layout stable.Two small points to consider:
Icon color override null-handling
For text, you only override whentextColor != null, but for icons you always callcopyWith(color: ExprOr.fromJson<String>(leadingIconColor))even when the color string is null. Depending on howIconProps.copyWithis implemented, that may either be fine or could inadvertently clear an existing color instead of leaving it unchanged.To mirror the text behavior more closely and make intent explicit:
final leadingIconProps =(localProps['leadingIcon'] as Map<String, Object?>?).maybe(IconProps.fromJson)?.copyWith(color: ExprOr.fromJson<String>(leadingIconColor));
final baseLeadingIconProps =(localProps['leadingIcon'] as Map<String, Object?>?).maybe(IconProps.fromJson);final leadingIconProps = leadingIconColor != null? baseLeadingIconProps?.copyWith(color: ExprOr.fromJson<String>(leadingIconColor)): baseLeadingIconProps;
final trailingIconProps =(localProps['trailingIcon'] as Map<String, Object?>?).maybe(IconProps.fromJson)?.copyWith(color: ExprOr.fromJson<String>(trailingIconColor));
final baseTrailingIconProps =(localProps['trailingIcon'] as Map<String, Object?>?).maybe(IconProps.fromJson);final trailingIconProps = trailingIconColor != null? baseTrailingIconProps?.copyWith(color: ExprOr.fromJson<String>(trailingIconColor)): baseTrailingIconProps;That keeps existing icon colors untouched when no override is supplied.
- Local props clone is only used for overrides, which is nice separation
UsinglocalPropsexclusively for override mutations (text color + icon colors) and still checkingprops.get('*.iconData')for existence keeps behavior clear and prevents false positives when icons are absent. This part of the refactor looks solid.Also applies to: 86-91, 105-110, 114-116, 125-155, 160-163
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/src/framework/widgets/button.dart (3)
37-40: Default state backgroundColor lacks null fallback.The disabled state has a fallback color (
Color(0xFFE0E0E0)), but the default state at line 40 could returnnullifevalColorreturns null. This may cause unexpected rendering behavior.Apply this diff to add a fallback:
final backgroundColor = isDisabled ? (payload.evalColor(disabledStyleJson.getString('backgroundColor')) ?? const Color(0xFFE0E0E0)) - : payload.evalColor(defaultStyleJson.getString('backgroundColor')); + : (payload.evalColor(defaultStyleJson.getString('backgroundColor')) ?? + Colors.blue); // or appropriate default color
76-80: Fix Size construction to avoid infinite dimensions.Using
double.infinityas a fallback for a missing dimension causes the button to expand infinitely in that direction, leading to layout overflow issues.Apply this diff to fix:
- fixedSize: (width != null || height != null) - ? WidgetStateProperty.all( - Size(width ?? double.infinity, height ?? double.infinity), - ) - : null, + fixedSize: (width != null && height != null) + ? WidgetStateProperty.all(Size(width, height)) + : null,If you need to support single-dimension constraints, use
minimumSizeormaximumSizeinstead offixedSize.
109-110: Performance concern: Avoid expensive JSON serialization for props cloning.Using
jsonDecode(jsonEncode(props.value))for deep cloning is computationally expensive and executes on every render. This could significantly impact performance when rendering many buttons.Consider a more efficient approach - only modify what's needed:
// Example: Clone only the text props if textColor override is needed final textProps = as$<JsonLike>(props.value['text']).maybe(TextProps.fromJson) ?? TextProps(); final modifiedTextProps = textColor != null ? textProps.copyWith(textStyle: textProps.textStyle?.copyWith(color: textColor)) : textProps;
🧹 Nitpick comments (1)
lib/src/framework/widgets/button.dart (1)
159-166: Consider filtering null icons from Row children.Using
SizedBox.shrink()for absent icons works but adds invisible widgets to the tree. A minor optimization would be to filter them out:return Row( mainAxisSize: MainAxisSize.min, - children: [ - leadingIcon ?? const SizedBox.shrink(), - Flexible(child: text), - trailingIcon ?? const SizedBox.shrink(), - ], + children: [ + if (leadingIcon != null) leadingIcon, + Flexible(child: text), + if (trailingIcon != null) trailingIcon, + ], );This avoids creating empty
SizedBoxwidgets when icons are absent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/framework/widgets/button.dart(3 hunks)
🔇 Additional comments (2)
lib/src/framework/widgets/button.dart (2)
42-52: LGTM!The per-state color derivation logic is clean and correctly distinguishes between disabled and default states for text and icon colors.
91-100: LGTM!The button properly disables interaction when
isDisabledis true by passingnulltoonPressed, and correctly triggers the action flow on press.
2 participants
No description provided.