Remove jQuery and replace lightbox with vanilla JS (Phase 1: Security & Performance)#158
Closed
lordfolken wants to merge 151 commits intomasterfrom
Closed
Remove jQuery and replace lightbox with vanilla JS (Phase 1: Security & Performance)#158lordfolken wants to merge 151 commits intomasterfrom
lordfolken wants to merge 151 commits intomasterfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (45)
📒 Files selected for processing (112)
📝 WalkthroughWalkthroughModernizes the Jekyll site: replaces legacy jQuery/Bootstrap-2 assets with Bootstrap 5, adds vanilla JS lightbox/carousel, syncs Google Play screenshots via a new script invoked in CI, refactors layouts/includes/CSS, restructures content (news/docs), and adds SEO/PWA/meta/config and linting/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as GitHub Actions (deploy)
participant Script as sync_google_play_metadata.rb
participant GitHub as GitHub API (repo/raw)
participant FS as Filesystem (repo)
participant Jekyll as Jekyll build
participant Deployer as rsync deploy
CI->>Script: execute pre-build script
Script->>GitHub: GET contents of phoneScreenshots (API)
GitHub-->>Script: return file list
Script->>GitHub: GET raw image blobs
GitHub-->>Script: stream image bytes
Script->>FS: write images to img/play-store/
Script->>FS: write _data/google_play_phone_screenshots.yml
Script-->>CI: exit 0
CI->>Jekyll: run bundle exec jekyll build --future
Jekyll->>FS: read data and templates (home-fastlane-carousel)
Jekyll-->>CI: produce _site/
CI->>Deployer: rsync _site/ -> production
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@css/lightbox.css`:
- Around line 54-80: Add a visible keyboard focus indicator for the lightbox
controls (.lightbox-close, .lightbox-prev, .lightbox-next) by defining a
:focus-visible rule that contrasts with the dark background (for example a white
or accent-colored outline or ring and increased box-shadow), ensure the outline
is not removed elsewhere so it appears only for keyboard focus (use
:focus-visible rather than :focus), and keep the existing sizing/positioning so
the focus ring does not shift layout.
In `@lib/lightbox.js`:
- Around line 51-62: The image loader lacks error handling: add an img.onerror
handler alongside img.onload in the lightbox code (the Image instance `img`) to
handle failed loads for `images[index]`; in the handler set a fallback state
(e.g., set `image.src` to a placeholder or clear it, set `image.alt` and
`caption.textContent` to an error message like "Image failed to load", update
`counter.textContent` appropriately, and adjust `prevBtn`/`nextBtn` visibility)
so the lightbox never stays blank when a network/404 error occurs.
- Around line 28-37: initLightbox currently inserts the lightboxHTML
unconditionally (document.body.insertAdjacentHTML) so calling initLightbox twice
creates duplicate overlays; fix by first checking for the existing lightbox
container (e.g., document.getElementById('lightbox-overlay') or another unique
id/class used by lightboxHTML) inside initLightbox and skip insertion if
present, or alternatively remove multiple init calls by combining selectors into
a single initLightbox call; update references in initLightbox and the
DOMContentLoaded/bootstrap area (the places that call initLightbox) to use the
chosen approach.
🧹 Nitpick comments (4)
WEBSITE_STRUCTURE.md (1)
15-15: Add language specifier to fenced code blocks.Per markdownlint, fenced code blocks should have a language specified for proper syntax highlighting.
Suggested fix
-``` +```text website/The same applies to the navigation structure block at line 248.
lib/lightbox.js (1)
103-118: Keyboard event listener is global and never removed.The
keydownlistener is attached todocumentand persists for the page lifetime. While this works, it processes key events even when the lightbox is closed (returning early after the display check). Consider attaching when opened and removing when closed for cleaner resource management._includes/header.html (1)
73-75: Consider renamingpage.jqueryto reflect its actual purpose.The front matter variable
page.jquerynow controls lightbox loading rather than jQuery loading. This naming is misleading after removing jQuery. Consider renaming to something likepage.lightboxfor clarity.This would require updating the front matter in pages that use the lightbox (e.g.,
index.html,discover/screenshots/index.html).Also applies to: 83-85
IMPROVEMENTS.md (1)
195-202: Note: Implementation differs from recommendation (which is fine).The document recommends GLightbox CDN, but the PR implements a custom vanilla JS solution instead. This is actually a good choice - it avoids external CDN dependency and keeps the implementation lightweight (~6KB vs external library). Consider updating this section to reflect the chosen approach.
Suggested update
### Replace jQuery Lightbox **Issue**: Issue `#42` already tracks this **Solution**: -```html -<!-- Replace with modern solution --> -<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/glightbox/dist/css/glightbox.min.css"> -<script src="https://cdn.jsdelivr.net/npm/glightbox/dist/js/glightbox.min.js"></script> -``` +Implemented with custom vanilla JS lightbox (`/lib/lightbox.js` and `/css/lightbox.css`) - no external dependencies, ~6KB total.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.htaccess:
- Line 6: The commented .htaccess CSP header is out-of-sync and misleading
because the active policy is set in the Content-Security-Policy meta tag inside
_includes/header.html; remove or update the commented header line in .htaccess
to avoid confusion and then make the CSP in _includes/header.html authoritative
(keep it in one place). If you want to tighten CSP, start by externalizing
inline styles/scripts referenced by the site (phpBB templates, download page map
styles, and main site element styling) and then remove 'unsafe-inline' from the
meta tag's script-src and style-src directives in _includes/header.html, testing
each change to ensure functionality remains.
🧹 Nitpick comments (2)
.htaccess (2)
14-15: Remove or replace the commentedX-XSS-Protectionheader. The header is deprecated and ignored by modern browsers; OWASP recommends relying on CSP instead. Either delete the line entirely or replace it with an explicitX-XSS-Protection: 0header for clarity if you want to document the decision to disable it.
5-24: PreferHeader always setto cover error responses.Without
always, headers are applied only to successful 2xx responses. For security headers (CSP, MIME sniffing prevention, clickjacking protection, XSS protection, Referrer Policy, Permissions Policy, and HSTS), usingHeader always setensures they are present even on 4xx/5xx error responses, preventing temporary security gaps on error pages.♻️ Suggested change (apply to all header lines)
-# Header set Content-Security-Policy "default-src 'self'; ..." +# Header always set Content-Security-Policy "default-src 'self'; ..."
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@_includes/header.html`:
- Around line 58-64: The meta-based X-* headers in _includes/header.html (the
meta http-equiv tags for X-Content-Type-Options, X-Frame-Options, and
X-XSS-Protection) give a false sense of protection because browsers ignore them
when set via meta; fix by moving these headers to server-level configuration:
uncomment and enable the equivalent directives in .htaccess (set
X-Content-Type-Options: nosniff, X-Frame-Options: SAMEORIGIN or proper CSP
frame-ancestors, and remove/replace X-XSS-Protection with a CSP fallback) so
server emits them consistently, and update SECURITY_HEADERS.md to reflect the
enabled server headers; alternatively, if you cannot enable server headers,
remove the meta http-equiv lines from _includes/header.html and document that
only CSP remains via meta in SECURITY_HEADERS.md.
In `@_layouts/trac.html`:
- Around line 14-18: Replace the jQuery DOM manipulation in the ready handler
with vanilla DOM APIs: select the footer element using the selector '#primary
`#footer`', remove it from its parent (preserve the node), then append that node
into the target container selected by '#secondary .row .col-12'; also select the
header via '#primary `#content` `#banner` `#header`' and hide it by setting its style
(e.g., display = "none"). Ensure this runs after DOMContentLoaded (use
DOMContentLoaded event instead of jQuery's ready) and handle missing elements by
checking for null before removing/appending or changing style.
In `@IMPROVEMENTS.md`:
- Line 22: Update the wording in IMPROVEMENTS.md to remove weak/redundant
phrases: replace "Bootstrap 2.x (very old, likely custom build)" with a more
precise phrase such as "Bootstrap 2.x (legacy, likely custom build)" and change
any instances of "SEO Optimization" to "SEO" (remove redundant "Optimization");
apply these edits at the occurrences referenced (the line shown plus the other
occurrences noted around lines 103 and 297) so the language is tighter and
non-redundant.
🧹 Nitpick comments (2)
css/lightbox.css (1)
108-112: Redundant selector — inlinedisplay: nonealready hides the element.This CSS rule targets elements that already have
display: noneset via inline styles, making the!importantoverride ineffective and the rule itself unnecessary. The JavaScript should handle visibility directly without needing this CSS fallback.Consider removing this block or using a class-based approach (e.g.,
.lightbox-nav-hidden) if you need CSS control over visibility.index.html (1)
95-134: Migratespan8tocol-md-8for consistency with the rest of the page.The
.span8class is still defined inlib/bootstrap/bootstrap-grid.css, but the page already usescol-md-8elsewhere (line 96). Standardizing oncol-md-8throughout would improve maintainability and consistency, especially given the mixed Bootstrap versions in use.♻️ Suggested update
- <div class="span8 news-item"> + <div class="col-md-8 news-item"> … - <div class="span8 news-item"> + <div class="col-md-8 news-item">
Comment on lines
14
to
18
| <script> | ||
| $(document).ready(function() { | ||
| $('#secondary .row .span12').append($('#primary #footer').detach()); | ||
| $('#secondary .row .col-12').append($('#primary #footer').detach()); | ||
| $('#primary #content #banner #header').hide(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if jQuery is actually included in the project's dependencies
if [ -f package.json ]; then
echo "=== Checking package.json ==="
cat package.json | head -50
fi
if [ -f Gemfile ]; then
echo "=== Checking Gemfile ==="
cat Gemfile | grep -i jquery || echo "No jQuery in Gemfile"
fi
echo ""
echo "=== Searching for jQuery includes/script tags ==="
rg -n 'jquery(\.min)?\.js|code\.jquery\.com|<script.*jquery' --type html || trueRepository: XCSoar/website
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check for jQuery usages across the codebase
echo "=== jQuery variable/function usages ==="
rg -n '\$\(' --type html -A 1 -B 1 || trueRepository: XCSoar/website
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Look at the specific file mentioned
echo "=== Full content of _layouts/trac.html ==="
if [ -f _layouts/trac.html ]; then
cat -n _layouts/trac.html
else
echo "File not found. Searching for trac.html..."
find . -name "trac.html" -type f
fiRepository: XCSoar/website
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Check the overall project structure to understand if this is a Jekyll site
echo "=== Project structure ==="
ls -la | head -20
echo ""
if [ -f _config.yml ]; then
echo "=== _config.yml (Jekyll config) ==="
cat _config.yml | head -30
fiRepository: XCSoar/website
Length of output: 1753
Convert to vanilla JavaScript; jQuery is not loaded and this script will fail at runtime.
The project has no jQuery includes (not in Gemfile or any layout files), but the code in _layouts/trac.html uses $() and .detach()/.hide() methods. This will throw a ReferenceError when the page loads, breaking the footer repositioning and header hiding logic.
🔧 Proposed vanilla JS replacement
<script>
- $(document).ready(function() {
- $('#secondary .row .col-12').append($('#primary `#footer`').detach());
- $('#primary `#content` `#banner` `#header`').hide();
- });
+ document.addEventListener('DOMContentLoaded', () => {
+ const footer = document.querySelector('#primary `#footer`');
+ const target = document.querySelector('#secondary .row .col-12');
+ if (footer && target) target.appendChild(footer);
+
+ const header = document.querySelector('#primary `#content` `#banner` `#header`');
+ if (header) header.style.display = 'none';
+ });
</script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <script> | |
| $(document).ready(function() { | |
| $('#secondary .row .span12').append($('#primary #footer').detach()); | |
| $('#secondary .row .col-12').append($('#primary #footer').detach()); | |
| $('#primary #content #banner #header').hide(); | |
| }); | |
| <script> | |
| document.addEventListener('DOMContentLoaded', () => { | |
| const footer = document.querySelector('#primary `#footer`'); | |
| const target = document.querySelector('#secondary .row .col-12'); | |
| if (footer && target) target.appendChild(footer); | |
| const header = document.querySelector('#primary `#content` `#banner` `#header`'); | |
| if (header) header.style.display = 'none'; | |
| }); | |
| </script> |
🤖 Prompt for AI Agents
In `@_layouts/trac.html` around lines 14 - 18, Replace the jQuery DOM manipulation
in the ready handler with vanilla DOM APIs: select the footer element using the
selector '#primary `#footer`', remove it from its parent (preserve the node), then
append that node into the target container selected by '#secondary .row
.col-12'; also select the header via '#primary `#content` `#banner` `#header`' and
hide it by setting its style (e.g., display = "none"). Ensure this runs after
DOMContentLoaded (use DOMContentLoaded event instead of jQuery's ready) and
handle missing elements by checking for null before removing/appending or
changing style.
| - **Impact**: Reduces page load, improves security, better performance | ||
|
|
||
| ### 2. **Outdated Bootstrap Version** | ||
| **Current**: Bootstrap 2.x (very old, likely custom build) |
There was a problem hiding this comment.
Tighten a couple of wording nits.
LanguageTool flags “very old” as a weak intensifier and “SEO Optimization” as redundant. Suggested tweaks below.
✏️ Suggested wording tweaks
-**Current**: Bootstrap 2.x (very old, likely custom build)
+**Current**: Bootstrap 2.x (outdated, likely custom build)
-### 8. **SEO Optimization**
+### 8. **SEO**
-| Medium | Medium | Medium | SEO optimization |
+| Medium | Medium | Medium | SEO |Also applies to: 103-103, 297-297
🧰 Tools
🪛 LanguageTool
[style] ~22-~22: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...p Version** Current: Bootstrap 2.x (very old, likely custom build) Issues: - No ...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
In `@IMPROVEMENTS.md` at line 22, Update the wording in IMPROVEMENTS.md to remove
weak/redundant phrases: replace "Bootstrap 2.x (very old, likely custom build)"
with a more precise phrase such as "Bootstrap 2.x (legacy, likely custom build)"
and change any instances of "SEO Optimization" to "SEO" (remove redundant
"Optimization"); apply these edits at the occurrences referenced (the line shown
plus the other occurrences noted around lines 103 and 297) so the language is
tighter and non-redundant.
lordfolken
force-pushed
the
modernize/remove-jquery-replace-lightbox
branch
from
9572488 to
198c66b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@_includes/download_sidebar.html`:
- Line 33: Remove the unintended double space before the English link in the
HTML snippet: find the line containing the anchor tag '<a href="{{
site.download_server_url }}/{{ site.xcsoar_stable_version
}}/XCSoar-manual.pdf">English</a>' in _includes/download_sidebar.html and change
"In <a ..." to "In <a ..." so there is only a single space between "In" and the
link.
In `@docs/index.md`:
- Line 18: The link text contains a duplicated word: replace the link label
"XCSoar XCSoar em um flash" with "XCSoar em um flash" (the link target is
XCSoar-in-a-flash-pt_BR.pdf), so update the markdown entry to remove the extra
"XCSoar" while keeping the existing flag and URL unchanged.
In `@IMPROVEMENTS.md`:
- Around line 5-20: Update the "Current: jQuery 1.8.2" label in IMPROVEMENTS.md
to indicate it's the pre-Phase-1 / legacy baseline (e.g., "Pre-Phase-1: jQuery
1.8.2") and add a short clarifying sentence that the live/deployed
implementation now uses the custom vanilla lightbox at /lib/lightbox.js (refer
to the "Implemented with custom vanilla JS" note) so readers understand the
document contrasts the legacy baseline with the current implemented state.
In `@index.html`:
- Around line 114-122: The carousel indicators list in the element with class
"carousel-indicators" is missing the button for the 8th slide; add a new
<button> with class "carousel-indicator", aria-label "Go to screenshot 8" (or
"Analysis: OLC" if you prefer more descriptive text) and data-slide="7" so the
indicator count matches the eight slides and navigation works correctly (follow
the pattern used by the existing "carousel-indicator" buttons).
In `@lib/screenshot-carousel.js`:
- Around line 28-33: The load handler for images can update the carousel width
even after the user has navigated away; modify the listener on activeImage so it
verifies the image is still the current active slide before calling
setCarouselWidth. Concretely, inside the 'load' callback registered on
activeImage, compare the event.target (or the captured activeImage) against the
current active slide image (e.g., query the carousel for the currently active
image or compare to a stored current reference) and only call
setCarouselWidth(activeImage) if they match; keep the { once: true } behavior to
avoid lingering listeners.
- Around line 127-139: The carousel's keydown handler won't run because the
carousel element isn't focusable; make the element referenced as carousel
focusable (e.g., set carousel.tabIndex = 0 or
carousel.setAttribute('tabindex','0')) before relying on the
addEventListener('keydown') so ArrowLeft/ArrowRight events fire and
prevSlide()/nextSlide() work; also consider adding an accessible label
(aria-label or role) for clarity and keep showSlide(0) initialization as-is.
In `@WEBSITE_STRUCTURE.md`:
- Around line 7-12: The doc still lists jQuery 1.8.2 and jquery-lightbox even
though the PR added a vanilla ES6+ lightbox; update WEBSITE_STRUCTURE.md to
remove references to jQuery and jquery-lightbox and document the new setup:
replace the "JavaScript: jQuery 1.8.2, custom scripts" line with "JavaScript:
Vanilla ES6+ lightbox, custom scripts" (and note no jQuery dependency) and
similarly change the other sections that mention jquery-lightbox (lines
referenced in the review) to describe the new ES6 module/lightbox implementation
and any initialization details or build/import steps so contributors aren’t
misled.
♻️ Duplicate comments (1)
_includes/header.html (1)
50-56: X- headers via<meta http-equiv>are not enforced by browsers.*Only
Content-Security-Policyis effective via<meta>tags. TheX-Content-Type-Options,X-Frame-Options, andX-XSS-Protectionmeta tags are ignored by modern browsers—these headers must be set server-side to have any effect.
🧹 Nitpick comments (10)
.cursor/rules/use-relative-links.mdc (1)
10-10: Minor inaccuracy in rationale.Root-relative paths don't actually avoid DNS lookups—DNS resolution happens at the domain level, regardless of path format. The actual performance benefit (if any) is avoiding potential redirects when the domain name changes. Consider removing or rewording this point.
Suggested fix
-- **Performance**: Slightly faster (no DNS lookup needed) +- **Performance**: Avoids potential redirects if domain configuration changesAlternatively, simply remove this bullet point since the performance difference is negligible.
lib/lightbox.js (1)
89-114: Image loading handles success and error cases correctly.The
onerrorhandler (lines 104-112) properly addresses the previous review concern about failed image loads. Navigation buttons remain functional even when an image fails.One minor enhancement to consider: clearing the image src before loading a new image could prevent a brief flash of the previous image during slow loads.
Optional enhancement
function showImage(index) { if (index < 0 || index >= allImages.length) return; currentIndex = index; + + // Clear previous image to avoid flash during load + image.src = ''; const img = new Image();_includes/header.html (1)
52-52: CSP usesunsafe-inlineandunsafe-evalwhich significantly weaken protection.The
'unsafe-inline'and'unsafe-eval'directives inscript-srcallow inline scripts andeval(), which defeats much of CSP's XSS protection. This appears necessary for the OpenLayers maps page that useseval()(ommaps.html line 121).Consider using nonce-based CSP or refactoring the maps page to avoid
eval()for stronger security. This could be tracked as a future improvement.css/xcsoar.css (7)
1-5: Consider the implications ofoverflow-x: hiddenon bothhtmlandbody.Applying
overflow-x: hiddento both elements prevents horizontal scrolling entirely, which can hide content from users who zoom in or use assistive technologies. If the intent is to prevent horizontal scroll caused by layout issues, consider fixing the root cause instead. If needed for specific components, apply it only to those containers.
98-103: Consider removing!importantif possible.The
!importantdeclarations suggest a specificity battle, likely with Bootstrap defaults. If feasible, increase selector specificity instead (e.g.,.navbar .navbar-nav .nav-link) to avoid cascading issues in future maintenance.
190-203: Mixed layout approaches: float-based and flexbox.This section uses
floatfor layout (lines 191-203), while the preceding section (lines 153-188) uses flexbox. The float-based styling with fixedremwidths may conflict with the responsive flexbox grid above. If this is legacy code for backward compatibility, consider adding a comment; otherwise, consolidate to one approach to avoid maintenance confusion.
314-325: Redundant padding declarations.Lines 315-317 set
padding: 0 1.25remand then immediately override withpadding-topandpadding-bottom. This can be simplified to a single declaration.♻️ Suggested fix
.carousel-slide figcaption { - padding: 0 1.25rem; - padding-top: 1.25rem; - padding-bottom: 1.25rem; + padding: 1.25rem; opacity: 0; transition: opacity 0.3s ease-in-out;
329-347: Missingcursor: pointeron interactive carousel buttons.The
.carousel-btnelements are clickable controls but lackcursor: pointer, which is a usability expectation for interactive elements. Same applies to.carousel-indicator(line 369).♻️ Suggested fix
.carousel-btn { position: absolute; top: 50%; transform: translateY(-50%); background: rgba(0, 0, 0, 0.6); color: `#fff`; border: 0.125rem solid `#fff`; border-radius: 50%; width: 3.125rem; height: 3.125rem; font-size: 2rem; line-height: 1; z-index: 10; display: flex; align-items: center; justify-content: center; padding: 0; transition: background-color 0.3s; + cursor: pointer; }And for the indicator:
.carousel-indicator { width: 0.75rem; height: 0.75rem; border-radius: 50%; border: 0.125rem solid `#878380`; padding: 0; transition: all 0.3s; + cursor: pointer; + background: transparent; }
439-455: Heavy use of!importantin footer styles.Multiple
!importantdeclarations suggest style override conflicts, likely with Bootstrap. While this works, it makes future maintenance harder. Consider scoping footer styles more specifically or using CSS custom properties for theming.
469-505: Potential list styling conflict.Line 471 sets
list-style-type: disconul, but line 504 setslist-style: noneon alllielements. This means the disc style fromulwill be overridden. If the intent is for most lists to have no markers except specific ones (lines 507-516), this works but is confusing. Consider adding a comment clarifying the cascade.
| - {% country_flag de %} [XCSoar Handbuch]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-manual-de.pdf) Complete German manual | ||
| - {% country_flag fr %} [XCSoar Prise en main]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-Prise-en-main.pdf) Quick guide in French | ||
| - {% country_flag fr %} [XCSoar Manuel de l'utilisateur]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-manual-fr.pdf) Mostly complete French manual | ||
| - {% country_flag br %} [XCSoar XCSoar em um flash]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-in-a-flash-pt_BR.pdf) Quick guide in Portuguese |
There was a problem hiding this comment.
Typo: Duplicate "XCSoar" in link text.
The link text reads "XCSoar XCSoar em um flash" with "XCSoar" appearing twice.
Proposed fix
-- {% country_flag br %} [XCSoar XCSoar em um flash]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-in-a-flash-pt_BR.pdf) Quick guide in Portuguese
+- {% country_flag br %} [XCSoar em um flash]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-in-a-flash-pt_BR.pdf) Quick guide in Portuguese📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - {% country_flag br %} [XCSoar XCSoar em um flash]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-in-a-flash-pt_BR.pdf) Quick guide in Portuguese | |
| - {% country_flag br %} [XCSoar em um flash]({{ site.download_server_url }}/{{ site.xcsoar_stable_version }}/XCSoar-in-a-flash-pt_BR.pdf) Quick guide in Portuguese |
🤖 Prompt for AI Agents
In `@docs/index.md` at line 18, The link text contains a duplicated word: replace
the link label "XCSoar XCSoar em um flash" with "XCSoar em um flash" (the link
target is XCSoar-in-a-flash-pt_BR.pdf), so update the markdown entry to remove
the extra "XCSoar" while keeping the existing flag and URL unchanged.
Comment on lines
114
to
122
| <div class="carousel-indicators"> | ||
| <button class="carousel-indicator active" aria-label="Go to screenshot 1" data-slide="0"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 2" data-slide="1"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 3" data-slide="2"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 4" data-slide="3"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 5" data-slide="4"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 6" data-slide="5"></button> | ||
| <button class="carousel-indicator" aria-label="Go to screenshot 7" data-slide="6"></button> | ||
| </div> |
There was a problem hiding this comment.
Bug: Indicator count doesn't match slide count.
There are 8 carousel slides but only 7 indicator buttons. The 8th slide ("Analysis: OLC") has no corresponding indicator, breaking navigation.
Suggested fix: Add missing indicator
<button class="carousel-indicator" aria-label="Go to screenshot 6" data-slide="5"></button>
<button class="carousel-indicator" aria-label="Go to screenshot 7" data-slide="6"></button>
+ <button class="carousel-indicator" aria-label="Go to screenshot 8" data-slide="7"></button>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="carousel-indicators"> | |
| <button class="carousel-indicator active" aria-label="Go to screenshot 1" data-slide="0"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 2" data-slide="1"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 3" data-slide="2"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 4" data-slide="3"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 5" data-slide="4"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 6" data-slide="5"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 7" data-slide="6"></button> | |
| </div> | |
| <div class="carousel-indicators"> | |
| <button class="carousel-indicator active" aria-label="Go to screenshot 1" data-slide="0"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 2" data-slide="1"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 3" data-slide="2"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 4" data-slide="3"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 5" data-slide="4"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 6" data-slide="5"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 7" data-slide="6"></button> | |
| <button class="carousel-indicator" aria-label="Go to screenshot 8" data-slide="7"></button> | |
| </div> |
🤖 Prompt for AI Agents
In `@index.html` around lines 114 - 122, The carousel indicators list in the
element with class "carousel-indicators" is missing the button for the 8th
slide; add a new <button> with class "carousel-indicator", aria-label "Go to
screenshot 8" (or "Analysis: OLC" if you prefer more descriptive text) and
data-slide="7" so the indicator count matches the eight slides and navigation
works correctly (follow the pattern used by the existing "carousel-indicator"
buttons).
Comment on lines
+28
to
+33
| // Wait for image to load if not already loaded | ||
| if (activeImage.complete) { | ||
| setCarouselWidth(activeImage); | ||
| } else { | ||
| activeImage.addEventListener('load', () => setCarouselWidth(activeImage), { once: true }); | ||
| } |
There was a problem hiding this comment.
Avoid width updates from non‑active slide image loads. If a previous slide finishes loading after the user navigates away, its load handler can override the width for the current slide. Guard the handler to only update when that slide is still active.
🐛 Proposed fix
- if (activeImage.complete) {
- setCarouselWidth(activeImage);
- } else {
- activeImage.addEventListener('load', () => setCarouselWidth(activeImage), { once: true });
- }
+ if (activeImage.complete) {
+ setCarouselWidth(activeImage);
+ } else {
+ const expectedIndex = currentSlide;
+ activeImage.addEventListener('load', () => {
+ if (expectedIndex === currentSlide) {
+ setCarouselWidth(activeImage);
+ }
+ }, { once: true });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Wait for image to load if not already loaded | |
| if (activeImage.complete) { | |
| setCarouselWidth(activeImage); | |
| } else { | |
| activeImage.addEventListener('load', () => setCarouselWidth(activeImage), { once: true }); | |
| } | |
| // Wait for image to load if not already loaded | |
| if (activeImage.complete) { | |
| setCarouselWidth(activeImage); | |
| } else { | |
| const expectedIndex = currentSlide; | |
| activeImage.addEventListener('load', () => { | |
| if (expectedIndex === currentSlide) { | |
| setCarouselWidth(activeImage); | |
| } | |
| }, { once: true }); | |
| } |
🤖 Prompt for AI Agents
In `@lib/screenshot-carousel.js` around lines 28 - 33, The load handler for images
can update the carousel width even after the user has navigated away; modify the
listener on activeImage so it verifies the image is still the current active
slide before calling setCarouselWidth. Concretely, inside the 'load' callback
registered on activeImage, compare the event.target (or the captured
activeImage) against the current active slide image (e.g., query the carousel
for the currently active image or compare to a stored current reference) and
only call setCarouselWidth(activeImage) if they match; keep the { once: true }
behavior to avoid lingering listeners.
Comment on lines
+127
to
+139
| // Keyboard navigation | ||
| carousel.addEventListener('keydown', function(e) { | ||
| if (e.key === 'ArrowLeft') { | ||
| prevSlide(); | ||
| } else if (e.key === 'ArrowRight') { | ||
| nextSlide(); | ||
| } | ||
| }); | ||
|
|
||
| // Make carousel focusable for keyboard navigation | ||
|
|
||
| // Initialize the first slide state on load | ||
| showSlide(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find HTML files that might contain the carousel
git ls-files | grep -E "\.(html|htm)$"Repository: XCSoar/website
Length of output: 576
🏁 Script executed:
# Search for the carousel element in the codebase
rg "screenshot-carousel" -S --max-count=20Repository: XCSoar/website
Length of output: 458
🏁 Script executed:
# Check the full context of the screenshot-carousel.js file
wc -l lib/screenshot-carousel.jsRepository: XCSoar/website
Length of output: 88
🏁 Script executed:
# Check the HTML markup for screenshot-carousel in index.html
rg -n "screenshot-carousel" index.html -B5 -A10Repository: XCSoar/website
Length of output: 885
🏁 Script executed:
# Read the full JavaScript file to understand the focusability setup
cat -n lib/screenshot-carousel.jsRepository: XCSoar/website
Length of output: 6564
The carousel lacks keyboard focusability — arrow-key navigation will not work. The keydown handler is attached (lines 127–134) but the carousel element itself is not focusable. Without tabindex="0" or equivalent, the div will never receive focus, so keyboard events won't fire. Add focusability in JavaScript to ensure accessibility:
Suggested fix
// Keyboard navigation
carousel.addEventListener('keydown', function(e) {
if (e.key === 'ArrowLeft') {
prevSlide();
} else if (e.key === 'ArrowRight') {
nextSlide();
}
});
// Make carousel focusable for keyboard navigation
+ if (!carousel.hasAttribute('tabindex')) {
+ carousel.setAttribute('tabindex', '0');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Keyboard navigation | |
| carousel.addEventListener('keydown', function(e) { | |
| if (e.key === 'ArrowLeft') { | |
| prevSlide(); | |
| } else if (e.key === 'ArrowRight') { | |
| nextSlide(); | |
| } | |
| }); | |
| // Make carousel focusable for keyboard navigation | |
| // Initialize the first slide state on load | |
| showSlide(0); | |
| // Keyboard navigation | |
| carousel.addEventListener('keydown', function(e) { | |
| if (e.key === 'ArrowLeft') { | |
| prevSlide(); | |
| } else if (e.key === 'ArrowRight') { | |
| nextSlide(); | |
| } | |
| }); | |
| // Make carousel focusable for keyboard navigation | |
| if (!carousel.hasAttribute('tabindex')) { | |
| carousel.setAttribute('tabindex', '0'); | |
| } | |
| // Initialize the first slide state on load | |
| showSlide(0); |
🤖 Prompt for AI Agents
In `@lib/screenshot-carousel.js` around lines 127 - 139, The carousel's keydown
handler won't run because the carousel element isn't focusable; make the element
referenced as carousel focusable (e.g., set carousel.tabIndex = 0 or
carousel.setAttribute('tabindex','0')) before relying on the
addEventListener('keydown') so ArrowLeft/ArrowRight events fire and
prevSlide()/nextSlide() work; also consider adding an accessible label
(aria-label or role) for clarity and keep showSlide(0) initialization as-is.
Comment on lines
+7
to
+12
| - **Static Site Generator**: Jekyll 4.4.1 | ||
| - **Ruby Version**: 3.4.8 | ||
| - **Theme**: Custom (not using a standard Jekyll theme) | ||
| - **CSS Framework**: Bootstrap (customized) | ||
| - **JavaScript**: jQuery 1.8.2, custom scripts | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if WEBSITE_STRUCTURE.md exists and read the relevant sections
find . -name "WEBSITE_STRUCTURE.md" -type fRepository: XCSoar/website
Length of output: 80
🏁 Script executed:
# Search for jQuery and jquery-lightbox references across the repo
rg "jquery-1\.8\.2|jquery-lightbox|jQuery.*lightbox" -i --no-headingRepository: XCSoar/website
Length of output: 4159
🏁 Script executed:
# Also check for any jQuery references more broadly to understand scope
rg "jQuery|jquery" -i --no-heading -c | head -20Repository: XCSoar/website
Length of output: 303
🏁 Script executed:
# Let's read the WEBSITE_STRUCTURE.md file to see current state at the mentioned lines
head -15 ./WEBSITE_STRUCTURE.mdRepository: XCSoar/website
Length of output: 509
🏁 Script executed:
# Check lines 87-92
sed -n '87,92p' ./WEBSITE_STRUCTURE.mdRepository: XCSoar/website
Length of output: 335
🏁 Script executed:
# Check line 106
sed -n '106p' ./WEBSITE_STRUCTURE.mdRepository: XCSoar/website
Length of output: 113
🏁 Script executed:
# Check lines 271-274
sed -n '271,274p' ./WEBSITE_STRUCTURE.mdRepository: XCSoar/website
Length of output: 250
🏁 Script executed:
# Verify if jQuery and jquery-lightbox are actually still in the codebase
ls -la lib/ | grep -E "jquery|lightbox"Repository: XCSoar/website
Length of output: 258
Update WEBSITE_STRUCTURE.md to reflect vanilla JS lightbox instead of jQuery. The documentation still lists jQuery 1.8.2 and jquery-lightbox across multiple sections (lines 7–12, 87–92, 106, 271–274), but the PR introduces a new vanilla ES6+ lightbox implementation. Update these sections to document the new setup and avoid misleading contributors.
🛠️ Example edits
- - **JavaScript**: jQuery 1.8.2, custom scripts
+ - **JavaScript**: Vanilla ES6+ (lightbox/carousel), custom scripts
- │ ├── jquery-1.8.2.min.js # jQuery
- │ └── jquery-lightbox/ # Lightbox plugin
+ │ ├── lightbox.js # Vanilla lightbox
+ │ └── screenshot-carousel.js # Screenshot carousel
- - **Features**: Screenshot gallery with jQuery lightbox
+ - **Features**: Screenshot gallery with vanilla JS lightbox
- - **JavaScript**: jQuery 1.8.2, custom scripts
- - **Libraries**: Bootstrap, jQuery Lightbox, country flags
+ - **JavaScript**: Vanilla ES6+, custom scripts
+ - **Libraries**: Bootstrap, lightbox, country flags🤖 Prompt for AI Agents
In `@WEBSITE_STRUCTURE.md` around lines 7 - 12, The doc still lists jQuery 1.8.2
and jquery-lightbox even though the PR added a vanilla ES6+ lightbox; update
WEBSITE_STRUCTURE.md to remove references to jQuery and jquery-lightbox and
document the new setup: replace the "JavaScript: jQuery 1.8.2, custom scripts"
line with "JavaScript: Vanilla ES6+ lightbox, custom scripts" (and note no
jQuery dependency) and similarly change the other sections that mention
jquery-lightbox (lines referenced in the review) to describe the new ES6
module/lightbox implementation and any initialization details or build/import
steps so contributors aren’t misled.
- Remove jQuery 1.8.2 (12+ years old, security risk) - Remove jQuery lightbox plugin - Add modern vanilla JS lightbox (no dependencies) - Update header.html to load new lightbox CSS/JS - Remove jQuery scripts from index.html and screenshots page - Remove unnecessary jquery: true from maps/waypoints pages Features of new lightbox: - No external dependencies - Keyboard navigation (Arrow keys, Escape) - Touch-friendly mobile support - Accessible (ARIA labels) - Smaller file size (~4KB vs ~90KB jQuery + plugin) - Modern ES6+ JavaScript Fixes #42 - Replace jquery lightbox with generic lightbox This is part of Phase 1: Critical Security & Performance improvements.
for more information, see https://pre-commit.ci
- Add Content-Security-Policy meta tag to prevent XSS attacks - Add X-Content-Type-Options to prevent MIME sniffing - Add X-Frame-Options to prevent clickjacking - Add X-XSS-Protection for legacy browser support - Add Referrer-Policy for privacy protection - Create .htaccess template for server-level headers - Add SECURITY_HEADERS.md documentation CSP Configuration: - Allows scripts from self, inline (for lightbox), and trusted CDNs - Allows styles from self, inline, and CDNs - Allows images from self, data URIs, and HTTPS sources - Allows connections to download.xcsoar.org, Google Maps, etc. - Blocks plugins (object-src 'none') - Restricts base-uri and form-action to self Security Headers: - Content-Security-Policy: Prevents XSS, data injection attacks - X-Content-Type-Options: Prevents MIME type confusion - X-Frame-Options: Prevents clickjacking - X-XSS-Protection: Legacy browser XSS protection - Referrer-Policy: Controls referrer information Note: Meta tags provide basic protection. For maximum security, HTTP headers should be configured at the server level (see .htaccess and SECURITY_HEADERS.md for configuration examples). This is part of Phase 1: Critical Security & Performance improvements.
…ness Fixes from CodeRabbit review: 1. Accessibility improvements: - Add :focus-visible styles for lightbox controls (keyboard navigation) - White outline with offset for clear focus indication on dark background 2. Error handling: - Add img.onerror handler for failed image loads - Display error message instead of blank lightbox - Maintain navigation functionality even on errors 3. Bug fixes: - Prevent duplicate lightbox HTML insertion - Combine selectors into single initLightbox call - Use shared state for all images across multiple selectors 4. Code quality: - Attach keyboard handler only when lightbox is open - Remove keyboard handler when lightbox is closed - Better resource management 5. Documentation and naming: - Rename page.jquery to page.lightbox for clarity - Update IMPROVEMENTS.md to reflect actual implementation - Add language specifiers to code blocks in WEBSITE_STRUCTURE.md All changes tested and build successful.
Major upgrade to modern Bootstrap version: 1. Replaced Bootstrap 2 CSS files with Bootstrap 5.3.8 CDN - Removed local bootstrap-normalize.css, bootstrap-button.css, bootstrap-grid.css - Added Bootstrap 5.3.8 from jsDelivr CDN with integrity hash 2. Updated grid system classes: - .span4 → .col-md-4 - .span8 → .col-md-8 - .span12 → .col-12 3. Updated utility classes: - .pull-right → .float-end - .btn-large → .btn-lg - .btn-block → .w-100 4. Updated all layout files: - _layouts/default.html, simple.html, post.html, develop.html - _layouts/download.html, discover.html, trac.html - _includes/header.html, footer.html, section_change.html - _includes/discover_menu.html, download_sidebar.html - index.html, discover/screenshots/index.html, discover/news.html 5. Updated custom CSS: - Changed #screenshots .span4 to #screenshots .col-md-4 Benefits: - Modern responsive grid system - Better mobile support - Improved accessibility - Latest security patches - Smaller CSS bundle (CDN cached) - Better browser compatibility Build tested and verified successful.
Create rule to enforce hosting all external resources (CSS, JS, fonts, images) locally instead of using CDN links. Benefits: - Better privacy and security (no external requests) - Improved reliability (no CDN dependency) - Offline support - Version control of all dependencies - Better performance (no external network latency) The rule is set to alwaysApply: true, so it will guide all future development to avoid CDN dependencies. Note: Bootstrap is currently loaded from CDN and should be migrated to local hosting per this rule.
Create rule to guide using native HTML5 features instead of JavaScript implementations whenever possible. Benefits: - Better performance (native browser features) - Improved accessibility (built-in ARIA support) - Progressive enhancement (works without JS) - Less code to maintain - Better SEO (semantic HTML) - Better mobile support The rule provides examples for: - Forms and input validation - Navigation and links - Modals and dialogs - Details/accordions - Images and lazy loading - Video/audio controls - Semantic HTML5 elements Rule is set to alwaysApply: true to guide all development.
Rule 1: Host all resources locally - Downloaded Bootstrap 5.3.8 CSS and source map - Moved from CDN to local: /lib/bootstrap-5.3.8/bootstrap.min.css - Updated header.html to use local Bootstrap path - Note: OpenLayers (maps) still uses CDN - separate task Rule 2: Prefer HTML5 over JavaScript - Added semantic <nav> element around navigation - Added lazy loading (loading="lazy") to all screenshot images - Converted screenshots to use <figure> and <figcaption> elements - Improved semantic HTML structure Benefits: - Better privacy (no Bootstrap CDN requests) - Improved performance (lazy loading images) - Better accessibility (semantic HTML5 elements) - Better SEO (semantic structure) - Progressive enhancement (works without JS) Files changed: - _includes/header.html (local Bootstrap, <nav>) - index.html (lazy loading, <figure>) - discover/screenshots/index.html (lazy loading, <figure>) - lib/bootstrap-5.3.8/ (new directory with Bootstrap files) Build tested and verified successful.
…utes HTML5 syntax improvements: - Remove self-closing slashes: <img /> → <img>, <br/> → <br> - Fix closing tag: </div> → </aside> for aside element - Add rel="noopener noreferrer" to external links with target="_blank" (security best practice to prevent window.opener attacks) Files updated: - index.html (br tags, img tags, aside closing, security rel) - discover/screenshots/index.html (br tags, img tags) - _includes/header.html (img tags) All changes follow HTML5 standard syntax (no XHTML-style self-closing tags). Build tested and verified successful.
Issue: After converting #primary from div to header, the main content was outside the white background container, causing text to flow outside. Solution: - Moved white background and width constraints to .shadowed container (which wraps both header and main) - Simplified #primary styling to just padding - Ensured main element is properly inside .shadowed container - Added proper closing of .shadowed div in footer Now both header and main content are contained within the white background area with proper width constraints (98% width, max 1230px). Files changed: - css/xcsoar.css (moved background to .shadowed, simplified #primary) - _includes/header.html (indentation fix) - _includes/footer.html (close .shadowed div) Build tested and verified successful.
Changed margin from 'margin: auto; margin-top: 1%' to explicit 'margin: 1% auto 0 auto' to ensure proper horizontal centering with 1% margin on each side (from width: 98%). This ensures the white content area has proper left/right margins on all screen sizes.
Changed from centered (margin: auto) to explicit margins: - Left margin: 2% (increased from 1%) - Right margin: 1% - Width: 97% (reduced from 98% to accommodate larger left margin) This gives more visual space on the left side of the content area.
Added padding: 0 20px to .shadowed container to create internal margins on left and right sides. This prevents content from touching the edges of the white container. The container now has: - External margins: 1% right, 2% left - Internal padding: 20px left and right - Content is properly spaced from container edges
Issues fixed: 1. Removed double padding - moved padding from .shadowed to .shadowed .container to avoid padding conflicts 2. Added logo image constraints - max-width: 100% to prevent cutoff 3. Reset logo container padding-left to 0 for proper left alignment The logo should now: - Be properly aligned on the left - Not be cut off on the right (responsive sizing) - Have consistent spacing with the rest of the header
Restored padding: 0 20px to .shadowed container to maintain internal left/right margins. Removed padding from .shadowed .container to avoid double padding, keeping only the .shadowed padding. This ensures: - External margins: 2% left, 1% right (outside white container) - Internal padding: 20px left/right (inside white container) - Content properly spaced from all edges
Issue: Logo was being cut off on the left side because container padding was removed, causing content to touch the edge. Fix: - Restored padding: 0 20px to .shadowed .container - Added overflow: visible to #logo to prevent clipping - Added width: auto to logo img to maintain natural size - Logo now has proper spacing and won't be cut off The logo (230px wide) should now display fully within the col-md-4 container with proper left/right spacing.
Issue: Logo SVG was missing viewBox attribute, causing content to be cut off on the left side when scaled or displayed. Fix: - Added viewBox="0 0 230 70" to logo-blue.svg - This ensures the SVG scales properly and all content is visible - Reverted container padding change (not the issue) The viewBox tells the browser the coordinate system and ensures the entire logo (0,0 to 230,70) is visible when rendered.
Added viewBox="0 0 230 70" to logo-green.svg and logo-red.svg to match the fix applied to logo-blue.svg. This ensures all logo variants scale properly and display correctly without cutoff.
Issue: The 'R' in XCSoar was being cut off because the SVG viewport was too narrow. Text starts at x=90px and extends to ~265px, but the SVG width was only 230px. Fix: - Increased width from 230px to 270px - Updated viewBox from "0 0 230 70" to "0 0 270 70" - Applied to all three logo variants (blue, green, red) Now the full 'XCSoar' text is visible when viewing the SVG directly or when it's rendered on the website.
Navigation changes: - Removed 'Hardware' from main navigation - Removed 'Contact' from main navigation - Removed 'Features' from discover menu - Simplified navigation to: Discover, Download, Contribute Homepage restructuring: - Created clear sections: About, Screenshots, News - Added semantic <section> elements for better structure - Reorganized content flow: About -> Screenshots -> News - Updated About section with content from discover/index.md - Improved News section layout with better headings - Removed SkyLines promotion (can be added elsewhere if needed) Benefits: - Cleaner, simpler navigation - Better homepage structure with clear sections - More focused user experience - Better semantic HTML5 structure
Reorganized homepage sections to flow correctly: 1. About section (with download sidebar) 2. Screenshots section 3. News section Removed duplicate news section and ensured proper semantic HTML5 structure with <section> elements.
Changed section order to: 1. Screenshots (visual first impression) 2. About (description with download sidebar) 3. News (latest updates) This puts the visual content first, which is more engaging for visitors landing on the homepage.
Navigation changes: - Removed 'Discover' from main navigation - Added 'Docs' heading that links to /discover/manual.html - Updated CSS hover styles (docs uses same blue as discover had) - Docs menu item is active when on discover pages The navigation now shows: Docs, Download, Contribute This makes it clearer that documentation is available and provides direct access to the manual.
Removed the entire aside element containing: - Download XCSoar button - Platform download links (Android, Windows, Linux, etc.) - Manual download links - Social media links The About section now uses full width (col-md-12) instead of col-md-8, giving more space for the content. Homepage is now cleaner and more focused on: Screenshots -> About -> News
Added a prominent button in the download sidebar linking to downloads.xcsoar.org/testing/ for the latest development build. The button uses warning style to indicate it's a testing version and includes a brief description.
Changed relative path ./ol.js to root-relative path /download/maps/ol.js in both index.html and ommaps.html to fix import failures. This ensures the OpenLayers JavaScript library loads correctly regardless of how the page is accessed.
Updated Content-Security-Policy to: - Add 'unsafe-eval' to script-src to allow eval() for maps.config.js parsing - Add worker-src 'self' blob: to allow blob workers used by OpenLayers This fixes CSP violations on the maps download page where: 1. eval() is used to parse the fetched maps.config.js file 2. OpenLayers creates blob workers for map rendering Also updated SECURITY_HEADERS.md documentation to reflect these changes.
- Fix unclosed fence, use bundle exec for Jekyll, align with CI Ruby and workflow - Document optional apt packages, --future, deployment on master - Clarify manual post creation and optional rake new_post (stg) - Refresh Gemfile.lock for wdm ~> 0.2.0 and current Bundler
- Move HTML5_ANALYSIS, IMPROVEMENTS, SECURITY_HEADERS, WEBSITE_STRUCTURE to docs/internal/ - Exclude docs/internal from Jekyll so only docs/index.md publishes under /docs/ - Move fabfile.py to _tools/ (use: fab -f _tools/fabfile.py render) - Refresh directory tree in WEBSITE_STRUCTURE.md
…pages Drop legacy lib/bootstrap fragments, xcsoar-not-trac overlay, download/data and waypoints pages, and retired blue/red full-wordmark SVGs superseded by the current navbar assets.
Serve lightbox from lib/lightbox/lightbox.js and lib/lightbox/lightbox.css; header references are updated in a follow-up commit with the rest of the asset pipeline.
Add woff2 cuts for site UI, document image provenance, green mark and red theme mark, and refresh the default xcsoar.svg used in the navbar.
Replace the old title row with a compact brand line (mark link, live wordmark, gray page title), Download dropdown (Releases / Maps), and a home-only tagline under the bar. Load DejaVu from /fonts/, tighten link and navbar rules, and refresh download page titles (e.g. Releases). Document HTML/CSS split in Cursor rules.
Add screenshots.css and optional carousel include, wire Fastlane phone shots under discover/screenshots/play-store, add news archive page, and sync metadata in CI via scripts/sync_google_play_metadata.rb. Extend _config for the Android package id and exclude scripts from the Jekyll site.
Refine CSP-facing markup on map download pages, align with local OL assets, and extract maps-specific layout rules into css/maps-page.css.
Update docs landing copy, maintainer structure/improvement notes, and fix outdated URLs in two release posts.
Align editor guidance with self-hosted fonts, lib paths, and in-repo links.
Point list-item backgrounds at icon-kobo, icon-pi, icon-cubie, and icon-debian SVGs (replacing missing PNG paths). Use list-item-cubie for Cubieboard links so Pi and Cubieboard rows show distinct icons.
Add jekyll-sitemap and set url/description in _config for sitemap.xml and absolute_url. Replace crawl-blocking robots.txt with Allow-all plus Sitemap line (Liquid-built). Emit per-page meta description from front matter, post excerpt, or site default; add link rel=canonical. Set concise descriptions on high-traffic pages.
Add site title and default og_image, emit og:* tags plus article:* on posts, and twitter:card summary_large_image with shared title/description/image. Support optional page.image for per-page share images.
…plication, BlogPosting, WebPage) Add seo-structured-data.html included from the main head: home graph for the project and app; BlogPosting for posts with publisher logo; WebPage for other documents. Values use jsonify for safe JSON.
Remove document-level CSP, X-Content-Type-Options, X-Frame-Options, and X-XSS-Protection; keep referrer policy. Hardening can live on the origin server if needed later.
Fill manifest fields with XCSoar for install prompts and OS launcher labels.
Drop forum-specific head/body branches from the shared header, forum templates, and xcsoar-forum.css. Delete standalone ommaps.html in favor of download/maps/index.html. Refresh WEBSITE_STRUCTURE.md accordingly.
- Remove discover/* (about, features, history, logos, screenshots, condor). - Permalink posts under /news/...; add root news.html and news-archive.html. - Move Play Store shots to img/play-store/; post images to img/attachments/; update _data YAML, sync script, carousel fallback link. - Docs: default layout; flags CSS on /docs; hardware section at page bottom. - Drop unused jQuery/lightbox, Trac/develop/discover layouts, xcsoar-trac.css, unused icons; update header nav and internal docs.
- Replace per-color stylesheets with xcsoar-theme.css and html.site-theme--*. - Drive main nav from _data/nav.yml via nav-main-items.html; set lang on html. - Default page color in header; trim layouts; post layout uses a single article. - Deduplicate page-title and :is(article, main) list rules in xcsoar.css. - Refresh WEBSITE_STRUCTURE.md; drop stale Discover wording in screenshots.css.
Add site-icons.html (favicon.ico, PNG favicons, Apple touch sizes, iOS web app meta, Windows tile config + TileImage, manifest link). Expand browserconfig.xml to full mstile set. Enrich site.webmanifest with id, start_url, scope, lang, description, categories, icon purpose.
- Extend .pre-commit-config.yaml with djlint (Jekyll/Liquid via jinja profile) and local stylelint for css/*.css. - Add .djlintrc and .stylelintrc.json; ignore .venv-precommit in .gitignore. - Trailing-whitespace hook: trim .cursor/rules/*.mdc.
Add renovate.yml (scheduled + workflow_dispatch, checkout fetch-depth 0, renovatebot/github-action v46.1.8). Token prefers RENOVATE_TOKEN else github.token. Set renovate.json enabled: true explicitly.
lordfolken
force-pushed
the
modernize/remove-jquery-replace-lightbox
branch
2 times, most recently
from
0005031 to
8f1fef8
Compare
for more information, see https://pre-commit.ci
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
1 participant
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.
Summary
This PR removes the outdated jQuery 1.8.2 dependency and replaces the jQuery lightbox with a modern vanilla JavaScript implementation.
Changes
Benefits
Features
The new lightbox includes:
Testing
Related Issues
Fixes #42 - Replace jquery lightbox with generic lightbox
Phase 1 Progress
This is part of Phase 1: Critical Security & Performance improvements:
File Size Comparison
Summary by CodeRabbit
Security
New Features
Bug Fixes
Documentation
Chores