feat(virtual-lab): improve subject nav and restructure home labs#983
Merged
Conversation
add subject-based hover menus with direct lab links move physics labs into a dedicated physics page normalize chemistry routes update home to subject cards + code editor entry
Contributor
WalkthroughReplaces static Virtual Lab navigation and home layout with a data-driven subject model, adds a Physics index page, consolidates Chemistry routes, and introduces build_virtual_lab_subjects() and render_virtual_lab_page() helpers; templates updated for i18n and accessible interactive dropdowns/cards. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant URLConf as URLConf (urls.py)
participant View as Django View (views.py)
participant Builder as build_virtual_lab_subjects()
participant Template as Template (home/layout/physics)
Browser->>URLConf: GET /virtual_lab/ or /virtual_lab/physics/
URLConf->>View: route -> virtual_lab_home / physics_home
View->>Builder: build_virtual_lab_subjects()
Builder-->>View: virtual_lab_subjects data
View->>Template: render(template, context with virtual_lab_subjects)
Template-->>Browser: HTML (dynamic nav, cards, dropdowns)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/virtual_lab/templates/virtual_lab/home.html`:
- Around line 17-45: The anchor cards lack explicit keyboard focus styles;
update each card anchor (the three subject anchors inside the if/elif/else
blocks referencing subject.url_name and the Code Editor anchor referencing
'virtual_lab:code_editor') to include accessible focus classes (e.g., add
focus:outline-none focus:ring-2 focus:ring-offset-2 with a color matching the
card's theme such as focus:ring-blue-300, focus:ring-green-300,
focus:ring-indigo-300, and focus:ring-teal-300 or the dark equivalents) so
keyboard users see a visible focus ring consistent with hover states.
In `@web/virtual_lab/templates/virtual_lab/layout.html`:
- Line 17: Top-level nav anchor elements using the class "hover:text-gray-900
dark:hover:text-white transition" (e.g., the link rendering "Home" and the other
top-level nav link around the same snippet) only define hover states; add
explicit keyboard focus styles (for example add focus:text-gray-900
dark:focus:text-white plus accessible focus outlines/rings such as
focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-[color]) so
keyboard users see the same visual treatment as hover. Update both occurrences
of the anchor with that class in the template (the top-level nav links) to
include these focus classes.
- Around line 19-31: The subject dropdown lacks ARIA semantics: update the
dropdown trigger (the <a> rendering {{ subject.label }}) to include
aria-haspopup="true", an id and aria-expanded bound to its open state (e.g.,
aria-expanded="false" initially), and aria-controls referencing the popup
container; mark the popup container (the div wrapping subject.labs) with
role="menu" and an id matching aria-controls, and set pointer elements (each lab
<a> in the {% for lab in subject.labs %}) to role="menuitem" and tabindex="-1"
so keyboard users can tab into the menu only when open; ensure the group/focus
behavior still toggles aria-expanded to "true" when visible.
In `@web/virtual_lab/templates/virtual_lab/physics/index.html`:
- Line 6: Replace the custom wrapper div that currently has class="max-w-5xl
mx-auto px-6 py-8" with the project-standard Tailwind container utility: update
the div (the page wrapper element with class "max-w-5xl mx-auto px-6 py-8") to
use "container mx-auto px-4" and preserve any vertical padding (e.g., keep
"py-8") so the final wrapper uses "container mx-auto px-4 py-8".
In `@web/virtual_lab/views.py`:
- Around line 15-53: Add explicit type hints: annotate
build_virtual_lab_subjects() with -> List[Dict[str, Any]] (and import List,
Dict, Any from typing) and annotate render_virtual_lab_page(request:
HttpRequest, template_name: str, extra_context: Optional[Dict[str, Any]] = None)
-> HttpResponse (import Optional from typing and HttpRequest/HttpResponse from
django.http). Update the function signatures for build_virtual_lab_subjects and
render_virtual_lab_page accordingly and add the necessary imports at the top of
the module.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
web/virtual_lab/templates/virtual_lab/home.htmlweb/virtual_lab/templates/virtual_lab/layout.htmlweb/virtual_lab/templates/virtual_lab/physics/index.htmlweb/virtual_lab/urls.pyweb/virtual_lab/views.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/virtual_lab/templates/virtual_lab/layout.html (1)
11-11: 🧹 Nitpick | 🔵 TrivialConsider using the standard container utility for header wrapper.
Line 11 uses
max-w-6xl mx-auto px-6instead of the project-standardcontainer mx-auto px-4. While this may be intentional for a narrower header, it's inconsistent with the guideline.As per coding guidelines, "Use Tailwind container classes: container mx-auto px-4 for containers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/virtual_lab/templates/virtual_lab/layout.html` at line 11, Replace the header wrapper's custom sizing classes with the project-standard Tailwind container classes: change the div whose class currently contains "max-w-6xl mx-auto px-6 py-4 flex items-center justify-between" to use "container mx-auto px-4 py-4 flex items-center justify-between" so it follows the guideline "container mx-auto px-4" while preserving padding, flex layout, and alignment in the element that wraps the header.
♻️ Duplicate comments (2)
web/virtual_lab/templates/virtual_lab/layout.html (2)
25-31: 🧹 Nitpick | 🔵 TrivialAdd
aria-haspopupto the dropdown trigger.The subject link controls a dropdown menu but is missing
aria-haspopup="menu"to indicate this to assistive technologies.♻️ Suggested fix
<a href="{% url subject.url_name %}" id="subject-menu-button-{{ forloop.counter }}" + aria-haspopup="menu" :aria-expanded="open ? 'true' : 'false'" aria-controls="subject-menu-{{ forloop.counter }}" class="hover:text-gray-900 dark:hover:text-white transition focus:text-gray-900 dark:focus:text-white focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-gray-400 dark:focus:ring-gray-300 dark:focus:ring-offset-gray-800">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/virtual_lab/templates/virtual_lab/layout.html` around lines 25 - 31, The anchor acting as the dropdown trigger (id "subject-menu-button-{{ forloop.counter }}") is missing the accessibility attribute aria-haspopup; update the <a> element that renders {{ subject.label }} to include aria-haspopup="menu" so assistive technologies recognize it controls a menu, keeping the existing aria-expanded and aria-controls attributes intact.
32-41: 🧹 Nitpick | 🔵 TrivialAdd
roleattributes for complete ARIA semantics on dropdown menus.The dropdown has
aria-labelledbybut is missingrole="menu"on the container androle="menuitem"on each lab link. This helps assistive technologies interpret the menu structure correctly.♻️ Suggested fix
<div id="subject-menu-{{ forloop.counter }}" aria-labelledby="subject-menu-button-{{ forloop.counter }}" + role="menu" class="absolute left-0 top-full pt-1 w-56 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 transition pointer-events-none group-hover:pointer-events-auto group-focus-within:pointer-events-auto z-20" :class="open ? 'opacity-100 pointer-events-auto' : 'opacity-0 pointer-events-none'"> <div class="bg-white dark:bg-gray-700 border border-gray-200 dark:border-gray-600 rounded-md shadow-lg"> {% for lab in subject.labs %} <a href="{% url lab.url_name %}" + role="menuitem" :tabindex="open ? 0 : -1" class="block px-4 py-2 text-gray-700 dark:text-gray-300 hover:bg-gray-100 dark:hover:bg-gray-600 transition">{{ lab.label }}</a> {% endfor %}As per coding guidelines, "Include proper ARIA labels where needed for accessibility."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/virtual_lab/templates/virtual_lab/layout.html` around lines 32 - 41, The dropdown container element with id "subject-menu-{{ forloop.counter }}" should include role="menu" and each lab link in the {% for lab in subject.labs %} loop should include role="menuitem"; update the container (the div that currently has aria-labelledby="subject-menu-button-{{ forloop.counter }}") to add role="menu" and update the anchor elements (the <a> elements rendering {{ lab.label }}) to add role="menuitem" so assistive technologies can correctly interpret the menu and its items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/virtual_lab/templates/virtual_lab/home.html`:
- Line 8: The container div at the top of virtual_lab/home.html uses nonstandard
classes ("max-w-5xl mx-auto px-6 py-8"); replace them with the project-standard
Tailwind container classes ("container mx-auto px-4") and preserve any necessary
vertical padding by adjusting or re-adding a py-* class if required so the
layout matches physics/index.html; update the class attribute on the <div
class="max-w-5xl mx-auto px-6 py-8"> element to use "container mx-auto px-4"
(and add py-8 only if the vertical spacing must be retained).
- Line 11: Wrap the hard-coded heading text inside Django's translation tag so
it matches the subtitle's i18n usage: replace the plain "Welcome to the Alpha
Science Lab" string within the <h2> element in
virtual_lab/templates/virtual_lab/home.html with a {% trans %} translation tag
(e.g., use {% trans "Welcome to the Alpha Science Lab" %} or {% trans %}...{%
endtrans %}) so the <h2> heading is localized consistently with the subtitle.
In `@web/virtual_lab/views.py`:
- Line 5: Replace the deprecated typing imports in web/virtual_lab/views.py:
remove Any, Dict, List, Optional from the from typing import ... line and update
type hints to use built-in generics and PEP 604 unions (e.g., use dict[..., ...]
instead of Dict[..., ...], list[...] instead of List[...], Any can be left as
builtins or replaced with object if appropriate, and Optional[T] -> T | None);
update all occurrences of Any, Dict, List, Optional in function signatures and
annotations (e.g., in view functions or class methods) to the corresponding
built-in forms so the file no longer imports from typing.
---
Outside diff comments:
In `@web/virtual_lab/templates/virtual_lab/layout.html`:
- Line 11: Replace the header wrapper's custom sizing classes with the
project-standard Tailwind container classes: change the div whose class
currently contains "max-w-6xl mx-auto px-6 py-4 flex items-center
justify-between" to use "container mx-auto px-4 py-4 flex items-center
justify-between" so it follows the guideline "container mx-auto px-4" while
preserving padding, flex layout, and alignment in the element that wraps the
header.
---
Duplicate comments:
In `@web/virtual_lab/templates/virtual_lab/layout.html`:
- Around line 25-31: The anchor acting as the dropdown trigger (id
"subject-menu-button-{{ forloop.counter }}") is missing the accessibility
attribute aria-haspopup; update the <a> element that renders {{ subject.label }}
to include aria-haspopup="menu" so assistive technologies recognize it controls
a menu, keeping the existing aria-expanded and aria-controls attributes intact.
- Around line 32-41: The dropdown container element with id "subject-menu-{{
forloop.counter }}" should include role="menu" and each lab link in the {% for
lab in subject.labs %} loop should include role="menuitem"; update the container
(the div that currently has aria-labelledby="subject-menu-button-{{
forloop.counter }}") to add role="menu" and update the anchor elements (the <a>
elements rendering {{ lab.label }}) to add role="menuitem" so assistive
technologies can correctly interpret the menu and its items.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
web/virtual_lab/templates/virtual_lab/home.htmlweb/virtual_lab/templates/virtual_lab/layout.htmlweb/virtual_lab/templates/virtual_lab/physics/index.htmlweb/virtual_lab/views.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/virtual_lab/templates/virtual_lab/home.html`:
- Around line 16-42: The template conditional chain should be replaced by
data-driven styling: update build_virtual_lab_subjects() to include a color (or
full class strings like bg_class, text_class, ring_class) on each subject dict
(e.g., "color": "blue" or "bg_class": "bg-blue-50 dark:bg-blue-900"), then
simplify the virtual_lab home template to interpolate those subject properties
(e.g., use subject.bg_class, subject.text_class, subject.ring_class) in the <a>
and <h3>/<p> class attributes instead of the if/elif/else block so adding new
subjects only requires adding metadata in build_virtual_lab_subjects().
In `@web/virtual_lab/views.py`:
- Around line 15-42: The subjects list is reconstructed on every request by
build_virtual_lab_subjects() which is called from render_virtual_lab_page();
make it static by either decorating build_virtual_lab_subjects with
functools.lru_cache(maxsize=1) or by computing the list once at module import
(e.g., VIRTUAL_LAB_SUBJECTS = build_virtual_lab_subjects()) and updating
render_virtual_lab_page to use that cached value — note gettext_lazy is safe for
caching so no translation issues.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/virtual_lab/templates/virtual_lab/home.htmlweb/virtual_lab/templates/virtual_lab/layout.htmlweb/virtual_lab/views.py
Comment on lines
+16
to
+42
| {% for subject in virtual_lab_subjects %} | ||
| {% if subject.key == "physics" %} | ||
| <a href="{% url subject.url_name %}" | ||
| class="block bg-blue-50 dark:bg-blue-900 border border-blue-200 dark:border-blue-700 rounded-lg p-6 shadow hover:shadow-md transition focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-300 dark:focus:ring-blue-400 dark:focus:ring-offset-gray-800"> | ||
| <h3 class="text-2xl font-semibold text-blue-700 dark:text-blue-300 mb-2">{{ subject.label }}</h3> | ||
| <p class="text-blue-600 dark:text-blue-400"> | ||
| {% blocktrans with count=subject.labs|length %}Explore {{ count }} labs{% endblocktrans %} | ||
| </p> | ||
| </a> | ||
| {% elif subject.key == "chemistry" %} | ||
| <a href="{% url subject.url_name %}" | ||
| class="block bg-green-50 dark:bg-green-900 border border-green-200 dark:border-green-700 rounded-lg p-6 shadow hover:shadow-md transition focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-green-300 dark:focus:ring-green-400 dark:focus:ring-offset-gray-800"> | ||
| <h3 class="text-2xl font-semibold text-green-700 dark:text-green-300 mb-2">{{ subject.label }}</h3> | ||
| <p class="text-green-600 dark:text-green-400"> | ||
| {% blocktrans with count=subject.labs|length %}Explore {{ count }} labs{% endblocktrans %} | ||
| </p> | ||
| </a> | ||
| {% else %} | ||
| <a href="{% url subject.url_name %}" | ||
| class="block bg-indigo-50 dark:bg-indigo-900 border border-indigo-200 dark:border-indigo-700 rounded-lg p-6 shadow hover:shadow-md transition focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-indigo-300 dark:focus:ring-indigo-400 dark:focus:ring-offset-gray-800"> | ||
| <h3 class="text-2xl font-semibold text-indigo-700 dark:text-indigo-300 mb-2">{{ subject.label }}</h3> | ||
| <p class="text-indigo-600 dark:text-indigo-400"> | ||
| {% blocktrans with count=subject.labs|length %}Explore {{ count }} labs{% endblocktrans %} | ||
| </p> | ||
| </a> | ||
| {% endif %} | ||
| {% endfor %} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider data-driven styling to reduce template conditionals.
The if/elif/else chain for subject-specific colors works but is verbose. If more subjects are added, this pattern will grow. Consider adding color classes to the subject data structure in build_virtual_lab_subjects().
♻️ Example approach
In views.py, add color metadata:
{
"key": "physics",
"label": _("Physics"),
"color": "blue", # or "bg_class": "bg-blue-50 dark:bg-blue-900", etc.
...
}Then simplify the template to use subject.color directly in class interpolation or a template filter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/virtual_lab/templates/virtual_lab/home.html` around lines 16 - 42, The
template conditional chain should be replaced by data-driven styling: update
build_virtual_lab_subjects() to include a color (or full class strings like
bg_class, text_class, ring_class) on each subject dict (e.g., "color": "blue" or
"bg_class": "bg-blue-50 dark:bg-blue-900"), then simplify the virtual_lab home
template to interpolate those subject properties (e.g., use subject.bg_class,
subject.text_class, subject.ring_class) in the <a> and <h3>/<p> class attributes
instead of the if/elif/else block so adding new subjects only requires adding
metadata in build_virtual_lab_subjects().
Comment on lines
+15
to
+42
| def build_virtual_lab_subjects() -> list[dict[str, object]]: | ||
| """Return subject and lab metadata for virtual lab navigation and landing pages.""" | ||
| return [ | ||
| { | ||
| "key": "physics", | ||
| "label": _("Physics"), | ||
| "url_name": "virtual_lab:physics_home", | ||
| "labs": [ | ||
| {"label": _("Pendulum Motion"), "url_name": "virtual_lab:physics_pendulum"}, | ||
| {"label": _("Projectile Motion"), "url_name": "virtual_lab:physics_projectile"}, | ||
| {"label": _("Inclined Plane"), "url_name": "virtual_lab:physics_inclined"}, | ||
| {"label": _("Mass-Spring Oscillation"), "url_name": "virtual_lab:physics_mass_spring"}, | ||
| {"label": _("Basic Electrical Circuit"), "url_name": "virtual_lab:physics_electrical_circuit"}, | ||
| ], | ||
| }, | ||
| { | ||
| "key": "chemistry", | ||
| "label": _("Chemistry"), | ||
| "url_name": "virtual_lab:chemistry_home", | ||
| "labs": [ | ||
| {"label": _("Acid-Base Titration"), "url_name": "virtual_lab:titration"}, | ||
| {"label": _("Reaction Rate"), "url_name": "virtual_lab:reaction_rate"}, | ||
| {"label": _("Solubility & Saturation"), "url_name": "virtual_lab:solubility"}, | ||
| {"label": _("Precipitation Reaction"), "url_name": "virtual_lab:precipitation"}, | ||
| {"label": _("pH Indicator"), "url_name": "virtual_lab:ph_indicator"}, | ||
| ], | ||
| }, | ||
| ] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching the subjects data structure.
build_virtual_lab_subjects() is called on every request via render_virtual_lab_page(). Since this data is static (only changes with code deployments), consider module-level initialization or @lru_cache to avoid repeated dictionary construction.
♻️ Suggested optimization
+from functools import lru_cache
+
+@lru_cache(maxsize=1)
def build_virtual_lab_subjects() -> list[dict[str, object]]:
"""Return subject and lab metadata for virtual lab navigation and landing pages."""
return [
# ...
]Note: gettext_lazy handles translation at render time, so caching the structure is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/virtual_lab/views.py` around lines 15 - 42, The subjects list is
reconstructed on every request by build_virtual_lab_subjects() which is called
from render_virtual_lab_page(); make it static by either decorating
build_virtual_lab_subjects with functools.lru_cache(maxsize=1) or by computing
the list once at module import (e.g., VIRTUAL_LAB_SUBJECTS =
build_virtual_lab_subjects()) and updating render_virtual_lab_page to use that
cached value — note gettext_lazy is safe for caching so no translation issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add subject-based hover menus with direct lab links
move physics labs into a dedicated physics page
normalize chemistry routes
update home to subject cards + code editor entry
Related issues
Fixes #982
Checklist
Summary by CodeRabbit
New Features
Refactor
UI