Responsive layout in Adjustments tab #4675
Conversation
WalkthroughConverted the adjustments UI for small screens into a card-like, stacked layout and added mobile-specific labels via CSS and data-label attributes; desktop styles remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant JS as adjustments.js
participant DOM as Browser DOM
Note right of JS: Creating a new adjustment entry
User->>JS: trigger add adjustment
JS->>DOM: create row elements (info, channelInfo, range, ...)
JS->>DOM: setAttribute(data-label) on each element
DOM-->>User: rendered card-like adjustment with CSS ::before labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (2)
src/css/tabs/adjustments.less (2)
107-109: Remove redundant background-color rule.The
&:nth-child(odd)selector at lines 107-109 applies the samebackground-color: var(--surface-200)as the parent.adjustmentrule at line 100, making it redundant.&:nth-child(odd) { - background-color: var(--surface-200); }
84-245: Consider accessibility implications of pseudo-element labels.The mobile layout uses CSS
::beforepseudo-elements to display labels viaattr(data-label). While this improves visual readability, screen readers typically cannot access pseudo-element content, so users with assistive technologies may not see the label associations with form controls. Consider complementing this approach with semantic HTML (e.g.,<label>elements) or ARIA attributes to ensure equal accessibility across all users.Would you like me to suggest an accessible labeling pattern that works alongside or replaces the pseudo-element approach?
|
|
Preview URL: https://pr4675.betaflight-app-preview.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/js/tabs/adjustments.js (1)
42-51: LGTM! Data-label attributes properly set for mobile responsive layout.The implementation correctly sets localized data-label attributes on the cloned adjustment elements, which the CSS will use via
attr(data-label)in::beforepseudo-elements for mobile labels. The placement after ID/index assignment is appropriate.For minor optimization, you could cache the
newAdjustmentjQuery object or chain multiple.find().attr()calls, though the current implementation prioritizes readability:// Optional: slightly more efficient const $adjustment = $(newAdjustment); $adjustment.find(".info").attr("data-label", i18n.getMessage("adjustmentsColumnEnable")); $adjustment.find(".channelInfo").attr("data-label", i18n.getMessage("adjustmentsColumnWhenChannel")); // ... etcsrc/css/tabs/adjustments.less (1)
206-224: LGTM! Function sections properly styled for mobile.Both
.functionSelectionand.functionSwitchChannelsections are appropriately styled with full-width selects and touch-friendly padding.For minor DRY improvement, you could combine the duplicate select styling:
.functionSelection, .functionSwitchChannel { select { width: 100%; padding: 0.5rem; border: 1px solid var(--surface-500); border-radius: 3px; box-sizing: border-box; } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/css/tabs/adjustments.less(1 hunks)src/js/tabs/adjustments.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/adjustments.js (1)
src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (5)
src/css/tabs/adjustments.less (5)
84-88: LGTM! Overflow change appropriate for mobile card layout.Changing overflow to
visibleis correct for the stacked card-based mobile layout, allowing cards to flow naturally without horizontal scrollbars.
90-96: LGTM! Standard responsive table transformation.Hiding the thead and making tbody block-level is the correct approach for converting the table to a stacked card layout on mobile, where each cell will display its own label via
data-labelattributes.
98-127: LGTM! Card layout and label display properly implemented.The card-based layout transformation is well-executed:
- Each
.adjustmentbecomes a standalone card with appropriate spacing and styling- Generic
td::beforepseudo-elements correctly display data-label attributes as block-level labels above content- The structure supports the responsive mobile experience
128-154: LGTM! Appropriate special treatment for the enable section.The
.infosection correctly receives different styling from other cells:
- Inline label display (lines 139-146) instead of block, allowing horizontal layout
- Flex row layout groups the "Enable" label with the checkbox for better UX
- Background color differentiation provides visual separation
This intentional override of the generic
td::beforestyling is appropriate for the enable toggle's different interaction pattern.
156-204: LGTM! Channel info and range sections properly adapted for mobile.The styling effectively handles the more complex elements:
.channelInfosection properly stacks the select dropdown and positions limit labels with flexbox (lines 169-188).rangesection adjusts slider margins and marker positioning (line 202) to accommodate the mobile card layout- Form controls are sized appropriately with full width and proper touch targets
2 participants
Made the rows of sliders in Adjustments tab responsive and adaptive to mobile.
Summary by CodeRabbit