Add Bootstrap 5 UI framework and modal-based demo output #11
Conversation
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
|
@copilot Failing ci |
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the Flask dashboard UI by integrating Bootstrap 5.3.2 and replacing inline demo output with modal dialogs. The implementation uses vanilla JavaScript for modal control while leveraging Bootstrap's CSS framework for styling and layout.
Key changes:
- Added Bootstrap 5.3.2 CSS and JS alongside existing Tailwind CSS
- Implemented modal dialog for demo execution output with loading states, error handling, and syntax highlighting
- Updated button styling throughout the dashboard with Bootstrap classes and custom gradients
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| flask_app/templates/base.html | Added Bootstrap 5 CSS/JS CDN links, Font Awesome icons, and demo output modal HTML structure |
| flask_app/templates/index.html | Replaced inline demo output with modal-based approach; updated button styling to use Bootstrap classes; implemented vanilla JS modal handlers with loading/error states |
| flask_app/static/style.css | Added comprehensive modal styling, updated CSS custom properties for improved theming, enhanced button styles with gradients, and refined card component aesthetics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flask_app/templates/base.html
Outdated
| }; | ||
| </script> | ||
| <script src="https://cdn.tailwindcss.com?plugins=forms"></script> | ||
| <!-- Bootstrap 5 CSS --> |
There was a problem hiding this comment.
The PR description mentions Bootstrap 5.3.0, but version 5.3.2 is being used. Please ensure this version mismatch is intentional or update the PR description to reflect the actual version being used.
| <!-- Bootstrap 5 CSS --> | |
| <!-- Bootstrap 5.3.2 CSS --> |
| function handleMenus(e) { | ||
| if (e.target.classList.contains('close-search')) { | ||
| hideSearchResults(); | ||
| } |
There was a problem hiding this comment.
The handleMenus function has been significantly reduced and appears to have lost functionality. The removed code handled:
- Menu trigger click events (
.menu-trigger) - Closing dropdown menus (
.dropdown-menu.show) - Close output button clicks (
.close-output)
Only the close-search functionality remains. If the menu trigger and dropdown functionality is no longer needed, this should be verified. If it is still needed elsewhere in the application, this could be a bug.
| function handleMenus(e) { | |
| if (e.target.classList.contains('close-search')) { | |
| hideSearchResults(); | |
| } | |
| function handleMenus(e) { | |
| // Handle close-search button | |
| if (e.target.classList.contains('close-search')) { | |
| hideSearchResults(); | |
| return; | |
| } | |
| // Handle menu trigger clicks | |
| if (e.target.classList.contains('menu-trigger')) { | |
| const dropdown = e.target.nextElementSibling; | |
| if (dropdown && dropdown.classList.contains('dropdown-menu')) { | |
| dropdown.classList.toggle('show'); | |
| } | |
| return; | |
| } | |
| // Handle closing dropdown menus | |
| if (e.target.classList.contains('dropdown-menu') && e.target.classList.contains('show')) { | |
| e.target.classList.remove('show'); | |
| return; | |
| } | |
| // Handle close output button clicks | |
| if (e.target.classList.contains('close-output')) { | |
| const output = e.target.closest('.output-container'); | |
| if (output) { | |
| output.style.display = 'none'; | |
| } | |
| return; | |
| } |
| if (!backdrop) { | ||
| backdrop = document.createElement('div'); | ||
| backdrop.className = 'modal-backdrop fade show'; | ||
| document.body.appendChild(backdrop); |
There was a problem hiding this comment.
Potential issue with backdrop management: If a demo is run multiple times, the code only checks if a backdrop exists before creating a new one, but doesn't remove the show class if it already exists. This could lead to stale backdrop state. Consider adding:
if (!backdrop) {
backdrop = document.createElement('div');
backdrop.className = 'modal-backdrop fade show';
document.body.appendChild(backdrop);
} else {
backdrop.className = 'modal-backdrop fade show';
}| document.body.appendChild(backdrop); | |
| document.body.appendChild(backdrop); | |
| } else { | |
| backdrop.className = 'modal-backdrop fade show'; |
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ module: moduleId }) | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing HTTP error handling: The fetch request doesn't check if response.ok is true. If the API returns a non-2xx status code, response.json() will still be called, which could fail or provide unexpected results. Consider adding:
const response = await fetch('/api/demo', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ module: moduleId })
});
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
const data = await response.json();| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } |
| // Set up modal close handlers on init | ||
| function setupModal() { | ||
| // Close button | ||
| const closeButtons = document.querySelectorAll('[data-bs-dismiss="modal"]'); | ||
| closeButtons.forEach(btn => { | ||
| btn.addEventListener('click', closeModal); | ||
| }); | ||
|
|
||
| // Clicking outside modal | ||
| const modal = document.getElementById('demoOutputModal'); | ||
| modal.addEventListener('click', function(e) { | ||
| if (e.target === modal) { | ||
| closeModal(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing null checks in setupModal: The function doesn't verify that the modal element exists before adding event listeners. If the modal HTML is not present on the page, this will cause a JavaScript error. Add a null check:
function setupModal() {
const modal = document.getElementById('demoOutputModal');
if (!modal) return;
// Close button
const closeButtons = document.querySelectorAll('[data-bs-dismiss="modal"]');
closeButtons.forEach(btn => {
btn.addEventListener('click', closeModal);
});
// Clicking outside modal
modal.addEventListener('click', function(e) {
if (e.target === modal) {
closeModal();
}
});
}
flask_app/templates/base.html
Outdated
| <!-- Bootstrap JS Bundle --> | ||
| <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.3.2/dist/js/bootstrap.bundle.min.js" integrity="sha384-C6RzsynM9kWDrMNeT87bh95OGNyZPhcTNXj1NW7RuBCsyN/o0jlpcV8Qyq46cDfL" crossorigin="anonymous"></script> | ||
|
|
There was a problem hiding this comment.
The PR description states "Integrated Bootstrap 5.3.0 CSS (no JS dependencies)" but Bootstrap JS Bundle is included on line 112. If the intention was to avoid Bootstrap JS dependencies and use vanilla JavaScript instead (as implemented), consider removing this script tag to avoid unnecessary bandwidth and potential conflicts, or update the PR description to reflect that Bootstrap JS is included.
| <!-- Bootstrap JS Bundle --> | |
| <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.3.2/dist/js/bootstrap.bundle.min.js" integrity="sha384-C6RzsynM9kWDrMNeT87bh95OGNyZPhcTNXj1NW7RuBCsyN/o0jlpcV8Qyq46cDfL" crossorigin="anonymous"></script> |
| if (e.target === modal) { | ||
| closeModal(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing keyboard accessibility: The modal doesn't handle the Escape key to close, which is a standard accessibility expectation for modal dialogs. Consider adding:
function setupModal() {
// ... existing code ...
// Escape key to close
document.addEventListener('keydown', function(e) {
const modal = document.getElementById('demoOutputModal');
if (e.key === 'Escape' && modal && modal.classList.contains('show')) {
closeModal();
}
});
}| }); | |
| }); | |
| // Escape key to close modal | |
| document.addEventListener('keydown', function(e) { | |
| const modal = document.getElementById('demoOutputModal'); | |
| if (e.key === 'Escape' && modal && modal.classList.contains('show')) { | |
| closeModal(); | |
| } | |
| }); |
| box-shadow: var(--shadow-md); | ||
| color: white; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Custom button styles (.btn-primary, .btn-success, etc.) completely override Bootstrap's built-in button styles. While this may be intentional for theming, it means Bootstrap button features like .disabled, .btn-sm size variations, and other modifiers may not work as expected. Consider either:
- Using Bootstrap's button customization via CSS variables, or
- Ensuring all Bootstrap button modifiers are also styled in the custom CSS, or
- Using more specific selectors to avoid complete override (e.g.,
.content-card .btn-primary)
This is particularly important for the .btn-sm classes used throughout the HTML which may not receive proper styling.
| /* Support for Bootstrap button modifiers with custom buttons */ | |
| .btn-success.btn-sm, | |
| .btn-secondary.btn-sm { | |
| padding: 0.25rem 0.5rem; | |
| font-size: 0.875rem; | |
| border-radius: 0.2rem; | |
| } | |
| .btn-success.disabled, | |
| .btn-success:disabled, | |
| .btn-secondary.disabled, | |
| .btn-secondary:disabled { | |
| opacity: 0.65; | |
| pointer-events: none; | |
| box-shadow: none; | |
| } |
| const moduleTitle = btn.dataset.title || 'Demo'; | ||
|
|
||
| // Get modal elements | ||
| const modal = document.getElementById('demoOutputModal'); | ||
| const modalTitle = document.getElementById('demoOutputModalLabel'); | ||
| const loadingSpinner = document.getElementById('demoLoadingSpinner'); | ||
| const outputContent = document.getElementById('demoOutputContent'); | ||
| const errorContent = document.getElementById('demoErrorContent'); | ||
| const outputCode = outputContent.querySelector('code'); | ||
| const errorCode = errorContent.querySelector('code'); | ||
|
|
||
| // Set modal title | ||
| modalTitle.textContent = `${moduleTitle} - Output`; |
There was a problem hiding this comment.
The modal title is set using textContent which is good, but the value comes from btn.dataset.title which is populated from the template with {{ d.title }}. Ensure that d.title is properly escaped in the Flask template context. If the title could contain user-generated content, this could be a potential XSS vector if not properly sanitized on the backend.
flask_app/templates/base.html
Outdated
| <!-- Bootstrap 5 CSS --> | ||
| <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.2/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-T3c6CoIi6uLrA9TneNEoa7RxnatzjcDSCmG1MXxSR1GAsXEV/Dwwykc2MPK8M2HN" crossorigin="anonymous"> |
There was a problem hiding this comment.
Both Tailwind CSS and Bootstrap 5 are loaded together, which can cause conflicts since both are comprehensive CSS frameworks with overlapping utility classes and reset styles. This can lead to:
- Increased page weight (both frameworks are large)
- CSS specificity conflicts
- Unexpected styling behavior
- Difficult debugging
Consider choosing one framework or explicitly scoping one of them. If both are truly needed, consider using Tailwind's prefix configuration or Bootstrap's Sass customization to namespace classes and avoid conflicts.
| <!-- Bootstrap 5 CSS --> | |
| <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.2/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-T3c6CoIi6uLrA9TneNEoa7RxnatzjcDSCmG1MXxSR1GAsXEV/Dwwykc2MPK8M2HN" crossorigin="anonymous"> |
|
@copilot apply changes based on the comments in this thread |
Applied all code review suggestions in commit 12e0769. Changes include: JavaScript improvements:
CSS improvements:
All 208 tests pass, pre-commit checks pass, and the modal works correctly with proper keyboard accessibility.
|
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
UX Upgrade Implementation
This PR implements a comprehensive UX upgrade for the Python DSA Demos Flask application with a modern CSS framework and enhanced user interactions.
Changes Made
1. UI Framework Integration
2. Modal Implementation
3. Visual Improvements
4. Responsive Design
5. CI Fixes
Technical Details
Testing
Files Modified
flask_app/templates/index.html- Added Bootstrap, modal HTML, updated JavaScriptflask_app/static/style.css- Enhanced styling, added modal CSSOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.