fix: remove duplicate nav bar in ScummVM packers + align style with D…#18
Open
aciderix wants to merge 1 commit into
Open
fix: remove duplicate nav bar in ScummVM packers + align style with D…#18aciderix wants to merge 1 commit into
aciderix wants to merge 1 commit into
Conversation
Reviewer's GuideRefactors the ScummVM packer EN/FR HTML pages to remove a duplicated top navigation bar and align their layout, styling, and progress/download UI with the existing DOS packer design while preserving existing functionality. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughComprehensive UI redesign of ScummVM packer HTML pages (English and French versions) with CSS reset, new header layout, reorganized navigation, refactored card and form components, redesigned dropzone and progress indicators, and updated JavaScript logic to align with new DOM structure. Core functionality preserved. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new dropzone markup removes the CSS that made the hidden file input cover the whole area (
position:absolute; inset:0; opacity:0;), and the added button doesn’t toggle the input, so the native file input may now be visible and clicking the drop area or button won’t behave as before—consider restoring the hidden-overlay input or wiring the button’s click to triggerfolder-input.click()and explicitly hiding the input. - In the embedded game template, the outer loading progress bar container was changed from
class="progress-bar"toclass="progress-fill", which likely breaks any styling or JS that expects a distinct track/container element; this looks like a typo and should probably remain a separate container class. - The progress step styling now uses
.progress-log .step.*selectors, but the generated<li>elements don’t have astepclass andsetStepActive/setStepDonetoggle onlyactive/done/erroron the<li>tags, so the CSS will never apply—either add astepclass to the<li>elements or change the selectors to target.progress-log li.active/.done/.error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new dropzone markup removes the CSS that made the hidden file input cover the whole area (`position:absolute; inset:0; opacity:0;`), and the added button doesn’t toggle the input, so the native file input may now be visible and clicking the drop area or button won’t behave as before—consider restoring the hidden-overlay input or wiring the button’s click to trigger `folder-input.click()` and explicitly hiding the input.
- In the embedded game template, the outer loading progress bar container was changed from `class="progress-bar"` to `class="progress-fill"`, which likely breaks any styling or JS that expects a distinct track/container element; this looks like a typo and should probably remain a separate container class.
- The progress step styling now uses `.progress-log .step.*` selectors, but the generated `<li>` elements don’t have a `step` class and `setStepActive/setStepDone` toggle only `active/done/error` on the `<li>` tags, so the CSS will never apply—either add a `step` class to the `<li>` elements or change the selectors to target `.progress-log li.active/.done/.error`.
## Individual Comments
### Comment 1
<location path="docs/pack_scummvm_game_fr.html" line_range="1306" />
<code_context>
<div id="loading">
<h1>🎮 {{GAME_TITLE}}</h1>
- <div class="progress-bar"><div class="progress-fill" id="progress"></div></div>
+ <div class="progress-fill"><div class="progress-fill" id="progress"></div></div>
<div class="status" id="status">Initializing...</div>
<div class="detail" id="detail"></div>
</code_context>
<issue_to_address>
**issue (bug_risk):** Same progress bar structure issue in the French template as in the English one.
This template should match the original loader structure: keep `progress-bar` on the outer div and `progress-fill` only on the inner div so existing CSS continues to apply correctly.
</issue_to_address>
### Comment 2
<location path="docs/pack_scummvm_game_en.html" line_range="238-120" />
<code_context>
-.dropzone .sublabel { font-size: 0.85em; color: var(--text-dim); }
-.dropzone input[type="file"] {
- position: absolute; inset: 0; opacity: 0; cursor: pointer; width: 100%; height: 100%;
+.dropzone-btn:hover {
+ border-color: var(--accent);
+ color: var(--accent);
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The hidden file input in the dropzone is no longer visually suppressed and may show alongside the custom button.
Previously, the file input was absolutely positioned and transparent so the native control was hidden while the dropzone remained clickable. With that styling removed, the browser’s default file input may now render alongside the custom button. If you’re triggering `folderInput.click()` programmatically, add styling to hide the input again (e.g. `display: none` or `position: absolute; opacity: 0;`) to prevent visual overlap with the custom UI.
Suggested implementation:
```
font-size: 0.85rem;
font-weight: 600;
cursor: pointer;
transition: all 0.2s;
}
/* Visually hide the native file input used for the folder picker while keeping it usable via JS */
.dropzone input[type="file"],
#folderInput {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: 0;
border: 0;
clip: rect(0, 0, 0, 0);
overflow: hidden;
opacity: 0;
pointer-events: none;
}
html { scroll-behavior: smooth; }
```
1. Ensure the file input used for folder selection either:
- Has `id="folderInput"`, or
- Is inside the `.dropzone` container so it matches `.dropzone input[type="file"]`.
2. Confirm that the JavaScript still calls `folderInput.click()` (or the correct element reference) so the hidden control is activated programmatically.
</issue_to_address>
### Comment 3
<location path="docs/pack_scummvm_game_fr.html" line_range="1660" />
<code_context>
const dlBtn = document.getElementById('download-btn');
dlBtn.href = url;
dlBtn.download = outputFilename;
+ document.getElementById('download-info').textContent = `🎮 ${engine} — 📝 ${gameTitle} — 📦 ${formatBytes(blob.size)} — ✈️ 100% offline`;
setStepDone(8);
</code_context>
<issue_to_address>
**nitpick:** Downloaded file info string in the French page is not localized (“100% offline”).
The `download-info` text on the French page still ends with `100% offline`, which is inconsistent with the localized UI. Consider translating this fragment (for example, to `100% hors ligne`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <div id="loading"> | ||
| <h1>🎮 {{GAME_TITLE}}</h1> | ||
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | ||
| <div class="progress-fill"><div class="progress-fill" id="progress"></div></div> |
There was a problem hiding this comment.
issue (bug_risk): Same progress bar structure issue in the French template as in the English one.
This template should match the original loader structure: keep progress-bar on the outer div and progress-fill only on the inner div so existing CSS continues to apply correctly.
|
|
||
| /* ── Tabs ── */ | ||
| .tabs { display: flex; gap: 4px; margin-bottom: 24px; border-bottom: 1px solid var(--border); } | ||
| /* Navigation Tabs */ |
There was a problem hiding this comment.
suggestion (bug_risk): The hidden file input in the dropzone is no longer visually suppressed and may show alongside the custom button.
Previously, the file input was absolutely positioned and transparent so the native control was hidden while the dropzone remained clickable. With that styling removed, the browser’s default file input may now render alongside the custom button. If you’re triggering folderInput.click() programmatically, add styling to hide the input again (e.g. display: none or position: absolute; opacity: 0;) to prevent visual overlap with the custom UI.
Suggested implementation:
font-size: 0.85rem;
font-weight: 600;
cursor: pointer;
transition: all 0.2s;
}
/* Visually hide the native file input used for the folder picker while keeping it usable via JS */
.dropzone input[type="file"],
#folderInput {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: 0;
border: 0;
clip: rect(0, 0, 0, 0);
overflow: hidden;
opacity: 0;
pointer-events: none;
}
html { scroll-behavior: smooth; }
- Ensure the file input used for folder selection either:
- Has
id="folderInput", or - Is inside the
.dropzonecontainer so it matches.dropzone input[type="file"].
- Has
- Confirm that the JavaScript still calls
folderInput.click()(or the correct element reference) so the hidden control is activated programmatically.
| const dlBtn = document.getElementById('download-btn'); | ||
| dlBtn.href = url; | ||
| dlBtn.download = outputFilename; | ||
| document.getElementById('download-info').textContent = `🎮 ${engine} — 📝 ${gameTitle} — 📦 ${formatBytes(blob.size)} — ✈️ 100% offline`; |
There was a problem hiding this comment.
nitpick: Downloaded file info string in the French page is not localized (“100% offline”).
The download-info text on the French page still ends with 100% offline, which is inconsistent with the localized UI. Consider translating this fragment (for example, to 100% hors ligne).
![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)
docs/pack_scummvm_game_fr.html (1)
1666-1668:⚠️ Potential issue | 🟡 MinorLocalization inconsistency: Error message uses English.
Line 1667 uses
'Error: 'instead of the French equivalent. This breaks the otherwise consistent French localization in this file.🌐 Proposed fix
} catch (err) { - setStepError(currentStep, 'Error: ' + err.message); + setStepError(currentStep, 'Erreur : ' + err.message); console.error(err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pack_scummvm_game_fr.html` around lines 1666 - 1668, The error message string is in English; update the call to setStepError so the prefix uses the French localization (replace "Error: " with "Erreur : ") when building the message from err.message (the call to setStepError(currentStep, 'Error: ' + err.message)); keep using currentStep and err.message so the error content is preserved and consistent with the French file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pack_scummvm_game_en.html`:
- Around line 1304-1309: The generated HTML uses the wrong class for the outer
progress container (it currently emits <div class="progress-fill">) which breaks
the CSS; update the template in packers/scummvm/pack_scummvm_game.py (the HTML
generation code around the loading block / the function/template that emits the
div with id="loading") so the outer container uses class="progress-bar" and the
inner remains <div class="progress-fill" id="progress"></div>, preserving the id
and inner structure expected by the stylesheet.
In `@docs/pack_scummvm_game_fr.html`:
- Around line 1304-1309: The progress bar outer element in the loading block
uses the wrong class name which breaks styling; change the outer div with
class="progress-fill" (the container surrounding the inner div with
id="progress") to class="progress-bar" so the structure matches the English
version and the CSS targets the correct element (look for the loading section
containing id="loading" and the inner div id="progress").
---
Outside diff comments:
In `@docs/pack_scummvm_game_fr.html`:
- Around line 1666-1668: The error message string is in English; update the call
to setStepError so the prefix uses the French localization (replace "Error: "
with "Erreur : ") when building the message from err.message (the call to
setStepError(currentStep, 'Error: ' + err.message)); keep using currentStep and
err.message so the error content is preserved and consistent with the French
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f34339a-3e76-4008-ba04-2ed82751c0aa
📒 Files selected for processing (2)
docs/pack_scummvm_game_en.htmldocs/pack_scummvm_game_fr.html
Comment on lines
1304
to
1309
| <div id="loading"> | ||
| <h1>🎮 {{GAME_TITLE}}</h1> | ||
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | ||
| <div class="progress-fill"><div class="progress-fill" id="progress"></div></div> | ||
| <div class="status" id="status">Initializing...</div> | ||
| <div class="detail" id="detail"></div> | ||
| </div> |
There was a problem hiding this comment.
Critical: Incorrect progress bar HTML structure in generated games.
The outer <div> at line 1306 uses class="progress-fill" but should be class="progress-bar". The CSS at lines 1293-1294 expects .progress-bar as the outer container (with width/height/background) and .progress-fill as the inner element. This mismatch causes the progress bar to be completely unstyled in generated game files.
As indicated in the context snippet from packers/scummvm/pack_scummvm_game.py:411-418, the correct structure is <div class="progress-bar"><div class="progress-fill" id="progress"></div></div>.
🐛 Proposed fix
<div id="loading">
<h1>🎮 {{GAME_TITLE}}</h1>
- <div class="progress-fill"><div class="progress-fill" id="progress"></div></div>
+ <div class="progress-bar"><div class="progress-fill" id="progress"></div></div>
<div class="status" id="status">Initializing...</div>
<div class="detail" id="detail"></div>
</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 id="loading"> | |
| <h1>🎮 {{GAME_TITLE}}</h1> | |
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | |
| <div class="progress-fill"><div class="progress-fill" id="progress"></div></div> | |
| <div class="status" id="status">Initializing...</div> | |
| <div class="detail" id="detail"></div> | |
| </div> | |
| <div id="loading"> | |
| <h1>🎮 {{GAME_TITLE}}</h1> | |
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | |
| <div class="status" id="status">Initializing...</div> | |
| <div class="detail" id="detail"></div> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pack_scummvm_game_en.html` around lines 1304 - 1309, The generated HTML
uses the wrong class for the outer progress container (it currently emits <div
class="progress-fill">) which breaks the CSS; update the template in
packers/scummvm/pack_scummvm_game.py (the HTML generation code around the
loading block / the function/template that emits the div with id="loading") so
the outer container uses class="progress-bar" and the inner remains <div
class="progress-fill" id="progress"></div>, preserving the id and inner
structure expected by the stylesheet.
Comment on lines
1304
to
1309
| <div id="loading"> | ||
| <h1>🎮 {{GAME_TITLE}}</h1> | ||
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | ||
| <div class="progress-fill"><div class="progress-fill" id="progress"></div></div> | ||
| <div class="status" id="status">Initialisation...</div> | ||
| <div class="detail" id="detail"></div> | ||
| </div> |
There was a problem hiding this comment.
Critical: Same progress bar HTML structure bug as English version.
Identical issue to the English file: the outer <div> uses class="progress-fill" instead of class="progress-bar", breaking the progress bar styling in generated French game files.
🐛 Proposed fix
<div id="loading">
<h1>🎮 {{GAME_TITLE}}</h1>
- <div class="progress-fill"><div class="progress-fill" id="progress"></div></div>
+ <div class="progress-bar"><div class="progress-fill" id="progress"></div></div>
<div class="status" id="status">Initialisation...</div>
<div class="detail" id="detail"></div>
</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 id="loading"> | |
| <h1>🎮 {{GAME_TITLE}}</h1> | |
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | |
| <div class="progress-fill"><div class="progress-fill" id="progress"></div></div> | |
| <div class="status" id="status">Initialisation...</div> | |
| <div class="detail" id="detail"></div> | |
| </div> | |
| <div id="loading"> | |
| <h1>🎮 {{GAME_TITLE}}</h1> | |
| <div class="progress-bar"><div class="progress-fill" id="progress"></div></div> | |
| <div class="status" id="status">Initialisation...</div> | |
| <div class="detail" id="detail"></div> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pack_scummvm_game_fr.html` around lines 1304 - 1309, The progress bar
outer element in the loading block uses the wrong class name which breaks
styling; change the outer div with class="progress-fill" (the container
surrounding the inner div with id="progress") to class="progress-bar" so the
structure matches the English version and the CSS targets the correct element
(look for the loading section containing id="loading" and the inner div
id="progress").
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.
…OS packer
Summary by Sourcery
Refresh the ScummVM packer HTML tools in both English and French to remove duplicated navigation and align their layout, styling, and progress/download UI with the DOS packer.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Documentation
Style