Refactor: Modernize Dashboard UI & Fix Theme Inconsistencies#222
Refactor: Modernize Dashboard UI & Fix Theme Inconsistencies#222Sai-Emani25 wants to merge 1 commit intoEswaramuthu:mainfrom
Conversation
|
@Sai-Emani25 is attempting to deploy a commit to the 007's projects Team on Vercel. A member of the Team first needs to authorize it. |
| color: var(--text-color); | ||
| width: 40px; | ||
| height: 40px; | ||
| border-radius: 50%; | ||
| cursor: pointer; | ||
| backdrop-filter: blur(10px); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| font-size: 1.2rem; | ||
| transition: all 0.3s ease; | ||
| padding: 0; | ||
| box-shadow: var(--shadow-sm); | ||
| } | ||
|
|
||
| #mode-toggle:hover { | ||
| background: var(--glass-bg); |
There was a problem hiding this comment.
Correctness: The #mode-toggle ID selector breaks responsive sizing because its higher specificity overrides the .theme-toggle media query rules at line 435. This prevents the button from scaling on mobile. Use the existing .theme-toggle class instead of an ID to maintain the responsive layout.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: static/styles.css. Lines 71-87 define #mode-toggle. This introduces a selector mismatch with existing .theme-toggle styles and media-query sizing. Please align the selector to reuse the existing .theme-toggle rules, or add equivalent responsive overrides for #mode-toggle in the media queries so mobile sizing remains consistent.
|
|
||
| # Inject Firebase config into all templates | ||
| @app.context_processor | ||
| def inject_firebase_config(): |
There was a problem hiding this comment.
Duplicate Code:
This function inject_firebase_config duplicates existing code.
📍 Original Location:
app.py:923 (student function), 994 (student_new function)
Function: Multiple inline implementations in route handlers (student, student_new)
💡 Recommendation:
Consolidation Recommendation:
-
Adopt the PR's approach: The
@app.context_processordecorator is the superior Flask pattern for this use case. It follows the DRY principle and automatically makesfirebase_configavailable to ALL templates without explicit passing. -
Refactor existing code: After adding the PR's
inject_firebase_config()function:- Remove
firebase_config = get_firebase_config()calls from lines 923 and 994 - Remove
firebase_config=firebase_configparameters from allrender_template()calls (lines 952, 953, 1049) - The templates will automatically receive
firebase_configthrough the context processor
- Remove
-
Keep the API endpoint: The
get_auth_firebase_config()function (lines 1392-1399) serves a different purpose (JSON API for client-side Firebase initialization) and should remain unchanged. -
Example of simplified code after consolidation:
# Before (current - line 922-953): @app.route("/student", methods=["GET", "POST"]) def student(): firebase_config = get_firebase_config() # REMOVE THIS # ... logic ... return render_template("student.html", firebase_config=firebase_config) # Remove parameter # After (with PR's context processor): @app.route("/student", methods=["GET", "POST"]) def student(): # ... logic ... return render_template("student.html") # firebase_config injected automatically
-
Benefits:
- Reduces code duplication (eliminates 2+ instances of manual config retrieval)
- Ensures consistency across all templates
- Easier to maintain (single source of truth)
- Automatically applies to any new templates without developer action
- Reduces chance of forgetting to pass firebase_config to a template
-
Impact: LOW risk - The context processor is a standard Flask pattern. The existing templates already expect
firebase_config, so no template changes are needed.
Consider importing and reusing the existing function instead of duplicating the logic.
There was a problem hiding this comment.
Pull request overview
This PR aims to modernize the dashboard/theme UI by introducing updated theme tokens (CSS variables), a unified icon-based theme toggle, and refreshed styling for the student achievements page.
Changes:
- Updated dark/light theme design tokens and added global styles for the theme toggle button.
- Refreshed
student_achievements_1.htmllayout/styles (cards, grid stats, spacing, responsive behavior). - Added a Flask
context_processorintended to injectfirebase_configinto all templates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| templates/teacher.html | Switches the theme toggle button to an icon-only control with a tooltip. |
| templates/student_achievements_1.html | Significant styling/layout modernization + icon-only theme toggle. |
| static/styles.css | Updates theme variables and adds global styling for .toggle-container / #mode-toggle. |
| app.py | Adds global template injection for Firebase config via @app.context_processor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .toggle-container { | ||
| position: fixed; | ||
| top: 20px; | ||
| right: 20px; | ||
| z-index: 1000; | ||
| } |
There was a problem hiding this comment.
.toggle-container is styled inline here, but the same rule set is now defined globally in static/styles.css. Keeping both copies increases the chance of theme/style drift; prefer removing this inline block and relying on the shared CSS (or intentionally overriding with only the differences).
|
|
||
| #mode-toggle { | ||
| background: var(--card-bg); | ||
| border: 1px solid var(--border-color); | ||
| color: var(--text-color); | ||
| width: 40px; | ||
| height: 40px; | ||
| border-radius: 50%; | ||
| cursor: pointer; | ||
| backdrop-filter: blur(10px); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| font-size: 1.2rem; | ||
| transition: all 0.3s ease; | ||
| padding: 0; | ||
| } | ||
|
|
||
| #mode-toggle:hover { | ||
| background: var(--glass-bg); | ||
| transform: rotate(15deg) scale(1.1); | ||
| border-color: var(--primary-color); | ||
| box-shadow: 0 0 15px var(--primary-color); | ||
| } |
There was a problem hiding this comment.
#mode-toggle styles are duplicated inline, but identical global styles were added to static/styles.css. Please remove the duplicated inline rules (or keep only page-specific overrides) so future theme tweaks don’t require editing multiple files.
| #mode-toggle { | |
| background: var(--card-bg); | |
| border: 1px solid var(--border-color); | |
| color: var(--text-color); | |
| width: 40px; | |
| height: 40px; | |
| border-radius: 50%; | |
| cursor: pointer; | |
| backdrop-filter: blur(10px); | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| font-size: 1.2rem; | |
| transition: all 0.3s ease; | |
| padding: 0; | |
| } | |
| #mode-toggle:hover { | |
| background: var(--glass-bg); | |
| transform: rotate(15deg) scale(1.1); | |
| border-color: var(--primary-color); | |
| box-shadow: 0 0 15px var(--primary-color); | |
| } |
| # Inject Firebase config into all templates | ||
| @app.context_processor | ||
| def inject_firebase_config(): | ||
| return dict(firebase_config=get_firebase_config()) | ||
|
|
There was a problem hiding this comment.
inject_firebase_config() is registered on the app instance created here, but later in this file app = Flask(__name__) is executed again (redefining the global app). That means this context processor will not apply to the exported/active app and templates still won’t receive firebase_config. Please ensure there is a single Flask app instance, or move/register the context processor on the final app object.
| # Inject Firebase config into all templates | ||
| @app.context_processor | ||
| def inject_firebase_config(): | ||
| return dict(firebase_config=get_firebase_config()) |
There was a problem hiding this comment.
This PR is described as a dashboard UI/theme refactor, but this change introduces a global template context variable for Firebase config (backend behavior change). Please either update the PR description/rationale to cover this, or move it to a separate PR so UI/theme work can be reviewed independently.
| #mode-toggle { | ||
| background: var(--card-bg); /* Use card-bg which is theme-aware */ | ||
| border: 1px solid var(--border-color); | ||
| color: var(--text-color); | ||
| width: 40px; | ||
| height: 40px; | ||
| border-radius: 50%; | ||
| cursor: pointer; | ||
| backdrop-filter: blur(10px); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| font-size: 1.2rem; | ||
| transition: all 0.3s ease; | ||
| padding: 0; | ||
| box-shadow: var(--shadow-sm); | ||
| } |
There was a problem hiding this comment.
The new global #mode-toggle style forces a 40x40 circular button, but many templates in the repo still render the toggle with text (e.g. "Dark Mode 🌙"). Without JS (or before it runs), that text will be clipped/overflow. Consider either updating remaining templates to icon-only toggles, or making the CSS accommodate both text and icon variants (e.g., allow auto width + padding when there’s text).
| # Inject Firebase config into all templates | ||
| @app.context_processor | ||
| def inject_firebase_config(): | ||
| return dict(firebase_config=get_firebase_config()) | ||
|
|
There was a problem hiding this comment.
New global template injection behavior (firebase_config in all templates) is introduced here, but the test suite doesn’t assert it. Adding a small route/template render test that confirms firebase_config is present (and contains required keys) would prevent regressions, especially since multiple routes currently pass it manually.
| <body> | ||
| <div class="toggle-container"> | ||
| <button id="mode-toggle">Dark Mode 🌙</button> | ||
| <button id="mode-toggle" title="Toggle Dark Mode">🌙</button> |
There was a problem hiding this comment.
The theme toggle button is now icon-only (🌙/☀️), but it has no accessible name. A title tooltip is not a reliable label for screen readers; please add an aria-label (and optionally manage aria-pressed / update the label based on current theme) so keyboard/screen-reader users can identify the control.
| <button id="mode-toggle" title="Toggle Dark Mode">🌙</button> | |
| <button id="mode-toggle" title="Toggle Dark Mode" aria-label="Toggle dark mode" aria-pressed="false">🌙</button> |
| <body> | ||
| <div class="toggle-container"> | ||
| <button id="mode-toggle">Dark Mode 🌙</button> | ||
| <button id="mode-toggle" title="Toggle Dark Mode">🌙</button> |
There was a problem hiding this comment.
The theme toggle button is icon-only (🌙/☀️) but lacks an accessible name. Please add an aria-label (and ideally reflect state via aria-pressed or by updating the label when the theme changes) so assistive technologies can announce the control correctly.
| <button id="mode-toggle" title="Toggle Dark Mode">🌙</button> | |
| <button id="mode-toggle" title="Toggle Dark Mode" aria-label="Toggle dark mode">🌙</button> |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?