Feat/frontend update#125
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFrontend styling and layout refinements for inputs, buttons, and grid layouts, plus navigation improvements on devices and MQTT log pages. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates input and button styling, adds several CSS utility classes and a wifi icon, and includes navigation improvements. Feedback highlights opportunities to improve CSS maintainability by removing redundant classes and !important flags, and suggests replacing inline styles with utility classes for better consistency.
| .u-width-auto { width: auto; } | ||
| .span-2 { grid-column: span 2; } | ||
| .span-full { grid-column: span 3; } | ||
| .summary-box--full { grid-column: span 3; } |
There was a problem hiding this comment.
The class .summary-box--full is redundant as it is identical to the existing .span-full class defined on the line above. Reusing existing utility classes helps keep the CSS file smaller and more maintainable.
| .accent-text { color: var(--accent); } | ||
| .accent-checkbox { accent-color: var(--accent); } | ||
| .u-danger-button { background: #ef4444; color: #fff; border: none; } | ||
| .u-ghost-button { background: var(--bg-widget) !important; border: 1px solid var(--border) !important; color: var(--text-secondary) !important; } |
There was a problem hiding this comment.
The use of !important is generally discouraged because it overrides the natural CSS cascade and makes future styling adjustments difficult. Since .u-ghost-button is defined after .md-button, it will naturally override the base styles when both classes are applied to the same element. Consider removing these flags.
.u-ghost-button { background: var(--bg-widget); border: 1px solid var(--border); color: var(--text-secondary); }| </div> | ||
|
|
||
| <div class="flex-row u-margin-top-2"> | ||
| <a href="{{ url_for('routes.index') }}" class="md-button u-ghost-button" style="width: auto; padding: 0.75rem 2rem;">Back to Dashboard</a> |
There was a problem hiding this comment.
Avoid using inline styles for layout and dimensions. You can use the existing .u-width-auto utility class instead of style="width: auto;". For the padding, consider defining a reusable utility class or a button modifier (e.g., .md-button--compact) to ensure consistency across the application.
<a href="{{ url_for('routes.index') }}" class="md-button u-ghost-button u-width-auto" style="padding: 0.75rem 2rem;">Back to Dashboard</a>
| <button class="md-button u-ghost-button" style="width: auto; padding: 0.75rem 2rem;" onclick="location.reload()">Refresh Log</button> | ||
| <a href="{{ url_for('routes.index') }}" class="md-button u-ghost-button" style="width: auto; padding: 0.75rem 2rem;">Back to Dashboard</a> |
There was a problem hiding this comment.
These buttons use duplicated inline styles. It is better to use utility classes like .u-width-auto and define a shared class for the specific padding to ensure a consistent UI and easier maintenance.
<button class="md-button u-ghost-button u-width-auto" style="padding: 0.75rem 2rem;" onclick="location.reload()">Refresh Log</button>
<a href="{{ url_for('routes.index') }}" class="md-button u-ghost-button u-width-auto" style="padding: 0.75rem 2rem;">Back to Dashboard</a>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider replacing the repeated inline button styles (width/padding/background/border) with shared utility or variant classes so the button appearance is controlled purely via CSS instead of templates.
- The new
.u-danger-buttonuses a hard-coded red color; aligning this with existing theme variables (e.g., a--dangertoken) would keep the palette consistent and make future theme changes easier. - The new
.summary-box--fullclass overlaps with the existing.span-fullgrid utility; consider reusing or extending the existing utility to avoid duplicating grid-span behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the repeated inline button styles (width/padding/background/border) with shared utility or variant classes so the button appearance is controlled purely via CSS instead of templates.
- The new `.u-danger-button` uses a hard-coded red color; aligning this with existing theme variables (e.g., a `--danger` token) would keep the palette consistent and make future theme changes easier.
- The new `.summary-box--full` class overlaps with the existing `.span-full` grid utility; consider reusing or extending the existing utility to avoid duplicating grid-span behavior.
## Individual Comments
### Comment 1
<location path="static/md3.css" line_range="259" />
<code_context>
-.u-ghost-button { background: var(--bg-widget) !important; color: var(--text-secondary) !important; }
+.accent-text { color: var(--accent); }
+.accent-checkbox { accent-color: var(--accent); }
+.u-danger-button { background: #ef4444; color: #fff; border: none; }
+.u-ghost-button { background: var(--bg-widget) !important; border: 1px solid var(--border) !important; color: var(--text-secondary) !important; }
.u-margin-left-auto { margin-left: auto; }
</code_context>
<issue_to_address>
**suggestion:** Align danger button colors with theme variables rather than hardcoded hex values.
Using `#ef4444` instead of a theme token makes this button harder to keep consistent across themes and future palette changes. Please hook this class up to the existing semantic danger/destructive color variable so it stays aligned with the design system.
Suggested implementation:
```
.u-danger-button { background: var(--color-danger); color: var(--color-on-danger); border: none; }
```
The exact variable names for the semantic danger colors may differ in your design system (common alternatives are `--danger`, `--danger-bg`, `--danger-foreground`, `--md-sys-color-error`, etc.). Please update `--color-danger` and `--color-on-danger` in this change to match the existing danger/destructive tokens defined elsewhere in `md3.css` or your global theme variables so that this class is fully aligned with your design system.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .u-ghost-button { background: var(--bg-widget) !important; color: var(--text-secondary) !important; } | ||
| .accent-text { color: var(--accent); } | ||
| .accent-checkbox { accent-color: var(--accent); } | ||
| .u-danger-button { background: #ef4444; color: #fff; border: none; } |
There was a problem hiding this comment.
suggestion: Align danger button colors with theme variables rather than hardcoded hex values.
Using #ef4444 instead of a theme token makes this button harder to keep consistent across themes and future palette changes. Please hook this class up to the existing semantic danger/destructive color variable so it stays aligned with the design system.
Suggested implementation:
.u-danger-button { background: var(--color-danger); color: var(--color-on-danger); border: none; }
The exact variable names for the semantic danger colors may differ in your design system (common alternatives are --danger, --danger-bg, --danger-foreground, --md-sys-color-error, etc.). Please update --color-danger and --color-on-danger in this change to match the existing danger/destructive tokens defined elsewhere in md3.css or your global theme variables so that this class is fully aligned with your design system.
|
@copilot Please address the comments from this code review: Overall Comments
Individual CommentsComment 1-.u-ghost-button { background: var(--bg-widget) !important; color: var(--text-secondary) !important; } +.accent-text { color: var(--accent); } +.accent-checkbox { accent-color: var(--accent); } +.u-danger-button { background: #ef4444; color: #fff; border: none; } +.u-ghost-button { background: var(--bg-widget) !important; border: 1px solid var(--border) !important; color: var(--text-secondary) !important; } .u-margin-left-auto { margin-left: auto; } **suggestion:** Align danger button colors with theme variables rather than hardcoded hex values.Using Suggested implementation: The exact variable names for the semantic danger colors may differ in your design system (common alternatives are |
…ss, add button--compact variant Agent-Logs-Url: https://github.com/DaTiC0/smart-google/sessions/0b2abbe2-8aef-4402-8ae2-5e46e1a6e7d0 Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
... Addressed all three points in commit
|
Summary
Validation
make test)make health)AI-Assisted Review (if applicable)
Risk & Rollback
Summary by Sourcery
Improve MD3 frontend styling consistency and navigation affordances across device and MQTT log pages.
Enhancements: