Modernize CSS and port to Bootstrap 5#107
Conversation
📝 WalkthroughWalkthroughReplaces legacy Bootstrap assets and grid; introduces a CSS-variable theme with dark mode, extensive styling/tokenization, accessibility tweaks, and responsive adjustments; modernizes templates (nav, sidebars, versions) with semantic markup, lazy-loading, safer external links, and replaces jQuery release logic with a module-based fetch and vanilla DOM updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Try not to lose the commit comments when merging, please. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_templates/sidebar_versions.html (1)
10-41: Guard release fetch + DOM updates to avoid runtime errors.If the API call fails or no non‑RC release exists, the script throws and the sidebar breaks. Also use
textContentinstead ofinnerHTMLfor safer DOM updates.🛡️ Suggested hardening
- const releases = await octokit.rest.repos.listReleases({ - owner: 'scikit-image', - repo: 'scikit-image', - per_page: 5, - page: 1 - }); + let releases; + try { + releases = await octokit.rest.repos.listReleases({ + owner: 'scikit-image', + repo: 'scikit-image', + per_page: 5, + page: 1 + }); + } catch (err) { + console.warn("Unable to load releases", err); + return; + } - let release = releases.data.filter(notRc)[0]; - let tag = release.tag_name; - let stable_version = tag.slice(1); + const release = releases.data.find(notRc); + if (!release) return; + const tag = release.tag_name; + const stable_version = tag.slice(1); - document.getElementById('stable-version').innerHTML = - stable_version + " - " + month_names[parseInt(month)-1] + " " + year; + const stableVersionEl = document.getElementById('stable-version'); + if (stableVersionEl) { + stableVersionEl.textContent = + stable_version + " - " + month_names[parseInt(month)-1] + " " + year; + } - var releaseNotesLink = document.getElementById('release-notes'); - var release_notes_url = releaseNotesLink.getAttribute('href') + "/release_" + stable_version_majmin + ".rst"; - releaseNotesLink.setAttribute('href', release_notes_url); + var releaseNotesLink = document.getElementById('release-notes'); + if (releaseNotesLink) { + var release_notes_url = releaseNotesLink.getAttribute('href') + "/release_" + stable_version_majmin + ".rst"; + releaseNotesLink.setAttribute('href', release_notes_url); + } - document.getElementById('dev-version').innerHTML = "pre-" + dev_version; + const devVersionEl = document.getElementById('dev-version'); + if (devVersionEl) { + devVersionEl.textContent = "pre-" + dev_version; + }
🧹 Nitpick comments (2)
_static/custom.css (1)
11-11: Transition only coverstransform, but hover also changesbackground-color.The hover state (line 18) also modifies
background-color, which will change abruptly without a transition. Consider including it:- transition: transform var(--transition-fast, 150ms ease-in-out); + transition: transform var(--transition-fast, 150ms ease-in-out), + background-color var(--transition-fast, 150ms ease-in-out);_templates/sidebar_versions.html (1)
1-4: Pin the Octokit CDN import to a specific version.Unpinned CDN dependencies can introduce breaking changes unexpectedly. Pin to a tested version and update intentionally. Use Octokit
5.0.5or specify your tested version:- import { Octokit } from "https://esm.sh/octokit"; + import { Octokit } from "https://esm.sh/octokit@5.0.5";
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_templates/sidebar_versions.html (1)
10-27: Guard against failed/empty release fetch to avoid runtime errors.If the release request fails or returns only RCs,
releasebecomesundefinedand the script throws, leaving the sidebar in a broken state. Add a safe fallback so the default placeholders remain intact and the rest of the page continues without errors.💡 Suggested fix (error handling + empty-release guard)
-const releases = await octokit.rest.repos.listReleases({ - owner: 'scikit-image', - repo: 'scikit-image', - per_page: 5, - page: 1 -}); - -const month_names = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"]; - -let release = releases.data.filter(notRc)[0]; -let tag = release.tag_name; -let stable_version = tag.slice(1); - -let date = release.published_at; -let year = date.substring(0, 4); -let month = date.substring(5, 7); -let day = date.substring(8, 10); +const month_names = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"]; + +try { + const releases = await octokit.rest.repos.listReleases({ + owner: 'scikit-image', + repo: 'scikit-image', + per_page: 5, + page: 1 + }); + + const release = releases.data.filter(notRc)[0]; + if (!release) { + console.warn("No non-RC releases found."); + } else { + const tag = release.tag_name; + const stable_version = tag.slice(1); + const date = release.published_at; + const year = date.substring(0, 4); + const month = date.substring(5, 7); + const day = date.substring(8, 10); + + // ... keep the rest of the DOM updates here ... + } +} catch (err) { + console.warn("Failed to load release metadata.", err); +}Also applies to: 31-40
🧹 Nitpick comments (1)
_templates/sidebar_versions.html (1)
28-40: PrefertextContentoverinnerHTMLfor version strings.These values are plain text, so
textContentavoids unnecessary HTML parsing and reduces injection risk if the upstream data ever changes.♻️ Suggested change
-document.getElementById('stable-version').innerHTML = +document.getElementById('stable-version').textContent = stable_version + " - " + month_names[parseInt(month)-1] + " " + year; ... -document.getElementById('dev-version').innerHTML = "pre-" + dev_version; +document.getElementById('dev-version').textContent = "pre-" + dev_version;
Modernize CSS: Upgrade to Bootstrap 5.3.3 and add dark mode This commit modernizes the website's frontend stack while preserving the current visual design and brand identity. Major changes: * Bootstrap upgrade (v2.1.1 → v5.3.3) - Replace 12-year-old Bootstrap with latest stable version - Remove jQuery dependency (Bootstrap 5 uses vanilla JavaScript) - Add CDN integrity checks for security - Remove legacy Bootstrap 2 CSS/JS files * Modern CSS architecture - Implement CSS custom properties (variables) for theming - Create design token system for spacing, colors, typography - Use modern font stacks with system fonts as fallbacks - Add consistent transitions and animations - Implement proper responsive breakpoints * Dark mode support - Auto-detect system preference via prefers-color-scheme - Complete dark color palette maintaining brand identity - Smooth transitions between light/dark modes - All components properly themed (navbar, sidebar, tables, code blocks) * WCAG 2.1 AA accessibility improvements - Add skip-to-content link for keyboard navigation - Implement ARIA labels and landmarks throughout - Add visible focus states (:focus-visible) on all interactive elements - Ensure 4.5:1 color contrast ratio for text - Add semantic HTML5 elements (nav, main, aside, header, footer) - Respect prefers-reduced-motion for accessibility - Add proper alt text and loading attributes to images * HTML/template updates - Convert Bootstrap 2 classes to Bootstrap 5 (span* → col-*, etc.) - Implement semantic HTML5 structure - Add mobile-first responsive navbar with hamburger menu - Update sidebar with Bootstrap 5 card components - Add proper ARIA attributes for screen readers * Team page modernization - Refactor team member cards with Flexbox - Add hover effects and transitions - Improve responsive behavior - Maintain circular photo styling * Performance improvements - Remove 30KB+ jQuery dependency - Add lazy loading for images - Use native CSS features instead of JavaScript - Optimize with print stylesheets * Browser compatibility - Chrome/Edge 88+ - Firefox 85+ - Safari 14+ - All modern mobile browsers Brand preservation: * Orange accent color (#CE5C00) maintained throughout * Current layout and visual hierarchy preserved * Logo and imagery unchanged Build tested successfully with Sphinx 9.0.4.
This commit updates the website styling with Bootstrap 5 patterns and fixes several visual issues. Changes: * Layout improvements - Switch from container-fluid to container for centered layout - Simplify navbar structure and use transparent background - Reduce vertical spacing between logo and navigation - Left-align navigation items properly * Hero section fixes - Fix h2 vertical alignment (was offset due to CSS specificity issue) - Make padding symmetrical between top and bottom - Add border and background to gallery-random image box - Remove excess margins from floated gallery element * Sidebar modernization - Convert version widget from jQuery to vanilla JavaScript - Update to Bootstrap 5 card components - Add SVG download icons with proper alignment - Style buttons with brand orange color * GitHub stars button - Replace ghbtns iframe with shields.io badge - Fixes JSONP/CORS errors in modern browsers - Style badge with brand orange (#CE5C00) * CSS cleanup - Remove old Bootstrap 2 CSS reference from theme.toml - Override Bootstrap button colors with brand orange - Add proper float clearing for section headings
stefanv
force-pushed
the
modernize-css
branch
from
8ea3c0a to
2a07fac
Compare
Refactor logo to use snake icon with HTML-rendered text, enabling proper dark mode color adaptation. Add Ubuntu font for logo text to match original branding. Fix dark mode contrast issues for navbar toggler icon, search input, and sidebar box text. Adjust header spacing for better visual balance.
stefanv
force-pushed
the
modernize-css
branch
from
2a07fac to
1d833a8
Compare
|
Was hoping we'd get CircleCI previews, but unfortunately not (I added it on main). I provided some screenshots instead. |
|
Looks great overall, thank you @stefanv! Perhaps, the order of sections in the mobile version could be made slightly different (if possible). More specifically, I would suggest "menu" -> "intro (skimage is a collection...)" -> "download" -> "other links". That would make the landing page a bit more user-friendly (I would assume people on mobile do not usually download the library anyway). |
|
Good point @soupault, I'll see what I can do. |
|
OK, I've swapped the sidebar and main column on mobile. |
The hover state changes both transform and background-color, but only transform was being transitioned, causing the background to snap abruptly.
These values are plain text, so textContent avoids unnecessary HTML parsing and reduces injection risk if the upstream GitHub API data ever changes.
Use inline SVG of the Zulip icon with currentColor fill so it matches the button text color. Style with flexbox for proper alignment.
- Add scikit-image logo and tagline to footer - Add centered copyright notice - Add Scientific Python logo linking to scientific-python.org - Use Ubuntu font for footer branding to match header - Fix capitalization of "Python" in header tagline
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_templates/sidebar_versions.html (1)
1-41:⚠️ Potential issue | 🟡 MinorMissing error handling for GitHub API calls.
The Octokit API call has no error handling. If the API fails (rate limiting, network issues), the version elements will display the placeholder text ("x.y.z") without any indication to users that data failed to load.
🛡️ Suggested fix with error handling
const octokit = new Octokit(); function notRc(release) { return !release.tag_name.includes('rc'); } +try { const releases = await octokit.rest.repos.listReleases({ owner: 'scikit-image', repo: 'scikit-image', per_page: 5, page: 1 }); const month_names = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"]; let release = releases.data.filter(notRc)[0]; + if (!release) { + console.warn('No non-RC releases found'); + return; + } let tag = release.tag_name; let stable_version = tag.slice(1); // ... rest of the code +} catch (error) { + console.error('Failed to fetch release information:', error); +}
🧹 Nitpick comments (5)
themes/scikit-image/static/css/theme.css (2)
202-225: Consider using CSS variables for logo colors.The
.logo-titleand.logo-taglineuse hardcoded color values (#333,#666) instead of the established CSS variables, which could lead to inconsistency.♻️ Suggested improvement
.logo-title { font-family: 'Ubuntu', var(--font-family-sans); font-size: 2.2rem; font-weight: 300; - color: `#333`; + color: var(--text-primary); line-height: 1.1; } .logo-tagline { font-family: 'Ubuntu', var(--font-family-sans); font-size: 1rem; font-weight: 300; - color: `#666`; + color: var(--text-secondary); } - -@media (prefers-color-scheme: dark) { - .logo-title { - color: `#e9ecef`; - } - - .logo-tagline { - color: `#adb5bd`; - } -}This eliminates the need for the separate dark mode override block at lines 217-225 since the variables already adapt.
464-479: Citation box colors are hardcoded.The
.citationclass uses hardcoded color values rather than CSS variables, which breaks the theming pattern established elsewhere.♻️ Suggested improvement
Consider adding citation-specific CSS variables to
:rootand the dark mode block::root { + /* Citation colors */ + --citation-text: `#004085`; + --citation-bg: `#cce5ff`; + --citation-border: `#b8daff`; + --citation-accent: `#007bff`; ... } `@media` (prefers-color-scheme: dark) { :root { + --citation-text: `#9ec5fe`; + --citation-bg: `#1c2a3a`; + --citation-border: `#2c5282`; + --citation-accent: `#2c5282`; ... } } .citation { - color: `#004085`; - background-color: `#cce5ff`; - border-color: `#b8daff`; - border-left: 4px solid `#007bff`; + color: var(--citation-text); + background-color: var(--citation-bg); + border-color: var(--citation-border); + border-left: 4px solid var(--citation-accent); padding: var(--space-lg); } - -@media (prefers-color-scheme: dark) { - .citation { - color: `#9ec5fe`; - background-color: `#1c2a3a`; - border-color: `#2c5282`; - } -}_templates/sidebar_versions.html (2)
31-40: Inconsistent variable declarations.The code mixes
let(lines 19-26) andvar(lines 31-38). For consistency and to benefit from block scoping, preferletorconstthroughout.♻️ Suggested fix
-var stable_version_majmin = stable_version.split(".").slice(0,2).join("."); -var releaseNotesLink = document.getElementById('release-notes'); -var release_notes_url = releaseNotesLink.getAttribute('href') + "/release_" + stable_version_majmin + ".rst"; +const stable_version_majmin = stable_version.split(".").slice(0,2).join("."); +const releaseNotesLink = document.getElementById('release-notes'); +const release_notes_url = releaseNotesLink.getAttribute('href') + "/release_" + stable_version_majmin + ".rst"; releaseNotesLink.setAttribute('href', release_notes_url); -var dev_version = stable_version.split(".").map(Number); +let dev_version = stable_version.split(".").map(Number); dev_version[1] += 1; dev_version = dev_version.slice(0, 2).join(".");
50-67: Duplicated SVG icons could be extracted.The download SVG icon is repeated twice (lines 51-54 and 62-65). Consider defining it once as a reusable template or symbol, though this is minor given there are only two instances.
themes/scikit-image/layout.html (1)
148-153: Consider addressing external asset dependencies with fallback or local hosting strategy.The Scientific Python logo URL (
cdn.jsdelivr.net) currently returns HTTP/2 200, but the page relies on multiple external CDN resources (Google Fonts, Bootstrap, Scientific Python logo) without fallback mechanisms. If any CDN becomes unavailable, assets will fail silently. Consider implementing a fallback strategy or hosting critical assets locally as part of a broader asset management approach.
- Wrap GitHub API call in try-catch and add fallback for missing releases. - Define CSS custom properties for logo and citation component colors. - Use same variable declaration `const` for all JS.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
themes/scikit-image/static/css/theme.css (3)
212-235: Logo colors should use the defined CSS variables.Lines 27-29 define
--logo-titleand--logo-taglinevariables, but they're never used. The.logo-titleand.logo-taglineclasses use hardcoded colors (#333,#666) instead, requiring a separate dark-mode media query block.♻️ Use the CSS variables for consistency
.logo-title { font-family: 'Ubuntu', var(--font-family-sans); font-size: 2.2rem; font-weight: 300; - color: `#333`; + color: var(--logo-title); line-height: 1.1; } .logo-tagline { font-family: 'Ubuntu', var(--font-family-sans); font-size: 1rem; font-weight: 300; - color: `#666`; + color: var(--logo-tagline); } -@media (prefers-color-scheme: dark) { - .logo-title { - color: `#e9ecef`; - } - - .logo-tagline { - color: `#adb5bd`; - } -}Then update the dark mode
:rootblock (lines 62-77) to include logo overrides:`@media` (prefers-color-scheme: dark) { :root { /* ...existing overrides... */ --logo-title: `#e9ecef`; --logo-tagline: `#adb5bd`; } }
474-489: Citation colors should use the defined CSS variables.Lines 32-35 define
--citation-text,--citation-bg,--citation-border, and--citation-accent, but the.citationclass uses hardcoded values instead. This duplicates the pattern seen with logo colors.♻️ Use the CSS variables
.citation { - color: `#004085`; - background-color: `#cce5ff`; - border-color: `#b8daff`; - border-left: 4px solid `#007bff`; + color: var(--citation-text); + background-color: var(--citation-bg); + border-color: var(--citation-border); + border-left: 4px solid var(--citation-accent); padding: var(--space-lg); } - -@media (prefers-color-scheme: dark) { - .citation { - color: `#9ec5fe`; - background-color: `#1c2a3a`; - border-color: `#2c5282`; - } -}Then add dark mode overrides in the
:rootdark mode block.
353-394: Button color overrides use redundant approaches.The code sets both Bootstrap 5 CSS custom properties (
--bs-btn-bg, etc.) and applies!importantoverrides on the same properties. While this ensures compatibility, it's redundant. Consider using just the Bootstrap variables if BS5 is the target.
- Fix top-level return in ES module (sidebar_versions.html) by restructuring to use conditional block instead of early return - Use CSS variables for logo and citation colors instead of hardcoded values, removing redundant dark mode media query blocks
- Add "This project is part of the scientific Python ecosystem" text next to the Scientific Python logo in the footer - Position chat button 1rem from screen edge instead of percentage - Add footer padding on mobile to prevent chat button overlap
Draft releases have `published_at: null`, causing an error when calling `.substring()` on the date.
2 participants
I've long wanted to modernize the style of the website, moving away from our ancient version of bootstrap.
This is a step in that direction. It makes no major alterations to content or layout, just uses more modern fonts, upgrades bootstrap, improves accessibility, and fixes a few random issues.
The content for this PR was generated by Claude.
Current:
Dark mode: missing
Mobile:
New version:
Dark mode:
Mobile:
Summary by CodeRabbit
New Features
Style
Accessibility
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.