Conversation
Coverage Report •
|
|||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable
The core change is straightforward - swapping widgets works fine. However, this feels like style over substance: ~40 lines of custom CSS just to make a checkbox look like a circle. The real question: does this actually improve UX, or are we over-engineering appearance?
Key Insight: Good code solves real problems. Is "switches feel less user-friendly than checkboxes" a real problem backed by user feedback, or just a preference? The implementation is clean, but the justification is weak.
Linus-Style Assessment
[IMPROVEMENT OPPORTUNITIES]
See inline comments below.
VERDICT: ✅ Worth merging with minor considerations - the code works, tests pass, nothing breaks. But consider whether the custom styling adds real value or just complexity.
| @property | ||
| def _button(self) -> Content: | ||
| button_style = self.get_visual_style("toggle--button") | ||
| if self.value: |
There was a problem hiding this comment.
🟡 Suggestion - Custom CSS Complexity: This RoundCheckbox class has ~40 lines of custom CSS just to render a circle (○) and checkmark (✔) instead of using Textual's default checkbox styling.
Question: Is this styling complexity necessary? The default Checkbox widget already works - this is pure appearance customization.
From a "good taste" perspective: "If the only problem is aesthetics, maybe it's not a problem." Consider whether the visual difference justifies the maintenance burden of custom styling code.
| self._label, value=self._value, id=self._switch_id, classes="form_switch" | ||
| ) | ||
| yield Static(self._description, classes="form_help switch_help") | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion - Layout Change Impact: This changes the layout from "Label: [Switch]" to "[○] Label" format.
Accessibility consideration:
- How does this behave with keyboard navigation (tab/space)?
- What do screen readers announce (is the label properly associated)?
- Is the Unicode ○/✔ announced correctly?
The PR description says "more user-friendly" but doesn't discuss accessibility. Before merging, verify this doesn't hurt keyboard-only or screen-reader users.
| def on_switch_changed(self, event: Switch.Changed) -> None: | ||
| """Handle switch changes to enable/disable threshold inputs.""" | ||
| if event.switch.id == "enable_iterative_refinement_switch": | ||
| def on_checkbox_changed(self, event: Checkbox.Changed) -> None: |
There was a problem hiding this comment.
🟢 Clean refactor: Event handler rename is consistent and straightforward. All query calls updated correctly. No issues here.
2 participants
Hey,
just a small change. Implementing checkboxes instead of switches.
I found them more user-friendly.
But let me know what you think ybout :-)
Before
After
🚀 Try this PR