added customization of Modern theme#1622
Merged
mambax7 merged 4 commits intoXOOPS:masterfrom Feb 25, 2026
Merged
Conversation
WalkthroughSplits Modern theme CSS into focused files (modern.css, xoops.css, fixes.css, custom.css), adds customization and developer docs, updates theme loader to include xoops.css and conditionally custom.css, trims modern.css to layout/components, and replaces literal emoji in templates with numeric references. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request introduces a major architectural improvement to the XOOPS Modern admin theme by implementing a five-file CSS cascade strategy and adding comprehensive customization support. The changes enable site administrators to safely customize the theme without their modifications being overwritten during updates, while also improving code organization and security.
Changes:
- CSS Architecture Refactoring: Transformed from a 3-file to a 5-file CSS strategy (modern.css, xoops.css, dark.css, fixes.css, custom.css), providing clear separation between theme components, XOOPS integration, dark mode overrides, core fixes, and user customizations
- Custom.css Support: Added conditional loading of custom.css file with comprehensive inline documentation, ensuring user customizations survive theme updates
- Security & Code Quality: Replaced deprecated
FILTER_SANITIZE_STRINGwithhtmlspecialchars(), improved menu.php with properincludeusage andisset()checks, converted emoji to HTML entities for encoding consistency - Documentation: Added comprehensive CUSTOMIZATION_GUIDE.md (400 lines) with recipes and best practices, updated DEVELOPER_GUIDE.md with new architecture diagrams, and enhanced README.md with customization instructions
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/modules/system/themes/modern/xotpl/xo_sidebar.tpl | Converted emoji to HTML entities (🏠→🏠, 🚪→🚪, 📊→📊, 📦→📦) for consistent rendering |
| htdocs/modules/system/themes/modern/xotpl/xo_head.tpl | Converted moon emoji to HTML entity 🌙, sun emoji left unchanged (minor inconsistency) |
| htdocs/modules/system/themes/modern/xotpl/xo_footer.tpl | Updated date format from strftime "%Y" to date "Y" format, consistent with other themes |
| htdocs/modules/system/themes/modern/modern.php | Added xoops.css loading, implemented conditional custom.css loading with file_exists check, replaced deprecated FILTER_SANITIZE_STRING with htmlspecialchars() + ENT_QUOTES | ENT_SUBSTITUTE |
| htdocs/modules/system/themes/modern/menu.php | Changed include_once to include for xoops_version.php (allows variable reinitialization), added isset() check for $modversion['hasAdmin'] |
| htdocs/modules/system/themes/modern/css/xoops.css | New file (857 lines) containing XOOPS admin element styles: message classes, tables, forms, tabs, toolbar, breadcrumb, logger output |
| htdocs/modules/system/themes/modern/css/modern.css | Refactored to contain only theme layout and components, removed ~450 lines of XOOPS-specific styles (moved to xoops.css), updated header comments |
| htdocs/modules/system/themes/modern/css/custom.css | New file with comprehensive inline documentation explaining usage, design tokens, and update-survival strategy |
| htdocs/modules/system/themes/modern/css/fixes.css | Updated header comments to reference new multi-file architecture |
| htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md | New comprehensive 400-line guide with recipes, troubleshooting, CSS file overview, and design token reference |
| htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md | Updated CSS architecture section from "Three-File Strategy" to "Five-File Strategy" with mermaid diagrams, corrected Smarty version to 4 |
| htdocs/modules/system/themes/modern/docs/*.md | Added XOOPS logo to all documentation files (DEVELOPER_GUIDE, USER_GUIDE, MODULE_INTEGRATION_GUIDE, CUSTOMIZATION_GUIDE) |
| htdocs/modules/system/themes/modern/README.md | Updated CSS Architecture section with five-file strategy table, added customization examples using custom.css |
htdocs/modules/system/themes/modern/docs/MODULE_INTEGRATION_GUIDE.md
Outdated
Show resolved
Hide resolved
htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/themes/modern/css/xoops.css`:
- Around line 830-833: Replace the hardcoded "80px" magic number with a design
token by adding a CSS variable --sidebar-collapsed-width: 80px to :root and
update all occurrences to use var(--sidebar-collapsed-width); specifically
change the rules that reference the collapsed sidebar width such as
body.sidebar-open `#xo-logger-output`, body.compact-sidebar .modern-sidebar, and
body.sidebar-open .modern-sidebar to use var(--sidebar-collapsed-width) so they
remain in sync and can be updated in one place.
- Around line 746-752: The global fieldset rule is too broad and should be
scoped to the admin theme container; replace the bare selector `fieldset` with a
scoped selector such as `.modern-layout fieldset` (or the theme’s root
container) so only fieldsets inside the modern theme are affected, retaining the
same properties (border, border-radius, padding, margin, background) in the rule
for `.modern-layout fieldset`.
- Around line 252-267: The global button selectors (input[type="submit"],
input[type="button"], button.formButton, .formButton) are too broad and may
override module/core styles; restrict them by scoping under the theme container
(e.g., prepend .modern-layout or .modern-theme) so only .modern-layout
input[type="submit"], .modern-layout input[type="button"], .modern-layout
button.formButton, .modern-layout .formButton are targeted — update the rule for
padding, margin-left, background, color, border, border-radius, font-size,
font-weight, cursor and transition accordingly to keep styling local to the
modern theme.
- Around line 226-250: The global form-element selectors (input[type="text"],
input[type="password"], input[type="email"], input[type="number"],
input[type="file"], select, textarea and their :focus variants) are too broad
and may override module content; scope them by prefixing with the
theme/container class or ID (e.g., .modern-layout or `#xo-content`) so the rules
only apply inside the theme layout; update both the base selector block and the
corresponding :focus block (referencing the exact selectors above) and apply the
same scoping change to any other broad form-element selectors in this CSS block.
In `@htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md`:
- Around line 1-2: Move the H1 "Modern Theme — Customization Guide" to the very
first line (put the XOOPS image after it), add language tags to the fenced code
blocks (use ```text for the file list and the modules path block) and ensure all
code fences are properly closed, and standardize spelling by replacing
“customise” with “customize” (e.g., in the sentence "This is the safest way to
customize one module without affecting others."); apply the same fixes to the
other occurrences referenced around the "modern.css / xoops.css / dark.css /
fixes.css / custom.css" listing and the later block near line 307 so all
headings, code fences and spelling are consistent.
In `@htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md`:
- Around line 1-2: The file has the image before the H1 which triggers MD041;
move the image markdown so the H1 "# Modern Admin Theme — Developer & Designer
Guide" is the very first line, and place the logo markdown ("") on the line
immediately after the H1 so the heading remains first and the image follows.
In `@htdocs/modules/system/themes/modern/docs/MODULE_INTEGRATION_GUIDE.md`:
- Around line 1-2: Move the H1 header to the very first line to satisfy
markdownlint MD041: put "# Module Integration Guide for Modern Theme" before the
image markdown (the current "") so the file
starts with the H1 and then the logo; keep the exact header text and image
unchanged aside from reordering.
In `@htdocs/modules/system/themes/modern/docs/USER_GUIDE.md`:
- Around line 1-2: Move the H1 "Modern Admin Theme — User Guide" to be the very
first line of the file to satisfy MD041; specifically, update the USER_GUIDE.md
so the first line is the heading "Modern Admin Theme — User Guide" and then
place the image banner "" immediately after
that heading.
In `@htdocs/modules/system/themes/modern/modern.php`:
- Around line 81-85: The file_exists check in modern.php uses XOOPS_PATH which
points to the library directory so it never finds the theme's custom.css; update
the existence check to use a relocatable filesystem path like __DIR__ .
'/css/custom.css' (keeping the stylesheet URL call
$xoTheme->addStylesheet(XOOPS_ADMINTHEME_URL . '/modern/css/custom.css')
unchanged) so file_exists(__DIR__ . '/css/custom.css') is used instead of
file_exists(XOOPS_PATH . '/modules/system/themes/modern/css/custom.css').
- Around line 100-102: filter_input() can return false (not null), so replace
the null-coalescing operator with the Elvis operator to treat false as falsy:
change the assignment of $darkMode (the line using filter_input(INPUT_COOKIE,
'xoops_dark_mode')) to use ?: instead of ?? so it falls back to '0' before
calling htmlspecialchars($darkMode,...); keep the subsequent htmlspecialchars
call intact.
In `@htdocs/modules/system/themes/modern/README.md`:
- Around line 1-2: The README currently starts with an image followed by the H1
which triggers MD041; move the heading "# XOOPS Modern Admin Theme" to the very
first line and place the logo image markdown () on the line(s)
immediately after the H1 so the heading is the document's first line and MD041
is satisfied.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
htdocs/modules/system/themes/modern/README.mdhtdocs/modules/system/themes/modern/css/custom.csshtdocs/modules/system/themes/modern/css/fixes.csshtdocs/modules/system/themes/modern/css/modern.csshtdocs/modules/system/themes/modern/css/xoops.csshtdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.mdhtdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.mdhtdocs/modules/system/themes/modern/docs/MODULE_INTEGRATION_GUIDE.mdhtdocs/modules/system/themes/modern/docs/USER_GUIDE.mdhtdocs/modules/system/themes/modern/menu.phphtdocs/modules/system/themes/modern/modern.phphtdocs/modules/system/themes/modern/xotpl/xo_footer.tplhtdocs/modules/system/themes/modern/xotpl/xo_head.tplhtdocs/modules/system/themes/modern/xotpl/xo_sidebar.tpl
Comment on lines
+226
to
+250
| input[type="text"], | ||
| input[type="password"], | ||
| input[type="email"], | ||
| input[type="number"], | ||
| input[type="file"], | ||
| select, | ||
| textarea { | ||
| padding: 8px 12px; | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius); | ||
| font-size: 14px; | ||
| transition: all 0.2s; | ||
| background: var(--bg-secondary); | ||
| } | ||
|
|
||
| input[type="text"]:focus, | ||
| input[type="password"]:focus, | ||
| input[type="email"]:focus, | ||
| input[type="number"]:focus, | ||
| select:focus, | ||
| textarea:focus { | ||
| outline: none; | ||
| border-color: var(--primary); | ||
| box-shadow: 0 0 0 3px rgba(37, 99, 235, 0.1); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unscoped form-element selectors may bleed into module content.
These selectors (input[type="text"], select, textarea, etc.) target every matching element on the page. Since admin pages can embed module content (iframes aside), these rules will override any module-specific form styling. Consider scoping them under a parent selector like .modern-layout or #xo-content to contain the blast radius.
♻️ Scoping suggestion (illustrative — apply to all broad selectors in this block)
-input[type="text"],
-input[type="password"],
-input[type="email"],
-input[type="number"],
-input[type="file"],
-select,
-textarea {
+.modern-layout input[type="text"],
+.modern-layout input[type="password"],
+.modern-layout input[type="email"],
+.modern-layout input[type="number"],
+.modern-layout input[type="file"],
+.modern-layout select,
+.modern-layout textarea {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 226 - 250,
The global form-element selectors (input[type="text"], input[type="password"],
input[type="email"], input[type="number"], input[type="file"], select, textarea
and their :focus variants) are too broad and may override module content; scope
them by prefixing with the theme/container class or ID (e.g., .modern-layout or
`#xo-content`) so the rules only apply inside the theme layout; update both the
base selector block and the corresponding :focus block (referencing the exact
selectors above) and apply the same scoping change to any other broad
form-element selectors in this CSS block.
Comment on lines
+252
to
+267
| /* Buttons */ | ||
| input[type="submit"], | ||
| input[type="button"], | ||
| button.formButton, | ||
| .formButton { | ||
| padding: 6px 16px; | ||
| margin-left: 8px; | ||
| background: var(--primary); | ||
| color: white; | ||
| border: none; | ||
| border-radius: var(--radius-sm); | ||
| font-size: 13px; | ||
| font-weight: 500; | ||
| cursor: pointer; | ||
| transition: all 0.2s; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Global button selectors (input[type="submit"], input[type="button"]) may override module and XOOPS core button styles unintentionally.
Same scoping recommendation as the form inputs — scope under .modern-layout or a similar container.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 252 - 267,
The global button selectors (input[type="submit"], input[type="button"],
button.formButton, .formButton) are too broad and may override module/core
styles; restrict them by scoping under the theme container (e.g., prepend
.modern-layout or .modern-theme) so only .modern-layout input[type="submit"],
.modern-layout input[type="button"], .modern-layout button.formButton,
.modern-layout .formButton are targeted — update the rule for padding,
margin-left, background, color, border, border-radius, font-size, font-weight,
cursor and transition accordingly to keep styling local to the modern theme.
Comment on lines
+746
to
+752
| fieldset { | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-lg); | ||
| padding: 20px; | ||
| margin: 20px 0; | ||
| background: var(--bg-secondary); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Global fieldset selector — same scoping concern as the form inputs above.
A bare fieldset rule will affect every <fieldset> on the page, including those rendered by third-party modules. Scope under .modern-layout fieldset or a similar admin-theme container.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 746 - 752,
The global fieldset rule is too broad and should be scoped to the admin theme
container; replace the bare selector `fieldset` with a scoped selector such as
`.modern-layout fieldset` (or the theme’s root container) so only fieldsets
inside the modern theme are affected, retaining the same properties (border,
border-radius, padding, margin, background) in the rule for `.modern-layout
fieldset`.
Comment on lines
+830
to
+833
| @media (min-width: 1025px) { | ||
| body.sidebar-open #xo-logger-output { | ||
| margin-left: 80px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Magic number 80px — use the design token or a dedicated variable.
The collapsed sidebar width 80px appears here and in several places in modern.css (e.g., body.compact-sidebar .modern-sidebar, body.sidebar-open .modern-sidebar). A --sidebar-collapsed-width: 80px variable in :root would keep these in sync and make future tweaks a one-liner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 830 - 833,
Replace the hardcoded "80px" magic number with a design token by adding a CSS
variable --sidebar-collapsed-width: 80px to :root and update all occurrences to
use var(--sidebar-collapsed-width); specifically change the rules that reference
the collapsed sidebar width such as body.sidebar-open `#xo-logger-output`,
body.compact-sidebar .modern-sidebar, and body.sidebar-open .modern-sidebar to
use var(--sidebar-collapsed-width) so they remain in sync and can be updated in
one place.
htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md
Outdated
Show resolved
Hide resolved
Comment on lines
1
to
2
|  | ||
| # Modern Admin Theme — Developer & Designer Guide |
There was a problem hiding this comment.
Keep the H1 as the first line to satisfy markdownlint.
Place the logo after the heading to avoid MD041.
📝 Suggested tweak
-
-# Modern Admin Theme — Developer & Designer Guide
+# Modern Admin Theme — Developer & Designer Guide
+
+📝 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
|  | |
| # Modern Admin Theme — Developer & Designer Guide | |
| # Modern Admin Theme — Developer & Designer Guide | |
|  |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md` around lines 1 -
2, The file has the image before the H1 which triggers MD041; move the image
markdown so the H1 "# Modern Admin Theme — Developer & Designer Guide" is the
very first line, and place the logo markdown ("") on the line
immediately after the H1 so the heading remains first and the image follows.
htdocs/modules/system/themes/modern/docs/MODULE_INTEGRATION_GUIDE.md
Outdated
Show resolved
Hide resolved
- Fix custom.css detection: use __DIR__ instead of XOOPS_PATH (wrong directory) - Fix dark mode cookie fallback: use ?: instead of ?? (filter_input returns false) - Replace raw sun emoji with HTML entities in xo_head.tpl - Remove literal "alt" from image alt text in all markdown docs - Fix US English spelling (customise→customize, colour→color) - Add language tags to bare code fences in CUSTOMIZATION_GUIDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
htdocs/modules/system/themes/modern/README.md (1)
1-2:⚠️ Potential issue | 🟡 MinorPut the H1 on the first line (MD041).
This is the same markdownlint issue noted earlier.🔧 Proposed change
- -# XOOPS Modern Admin Theme +# XOOPS Modern Admin Theme +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/README.md` around lines 1 - 2, Move the H1 so it is the very first line of README.md to satisfy markdownlint MD041: place the line "# XOOPS Modern Admin Theme" above the image (currently ""), ensuring the H1 text remains exactly "XOOPS Modern Admin Theme" and the image follows immediately after.htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md (1)
1-2:⚠️ Potential issue | 🟡 MinorPlace the H1 on the first line (MD041).
Same issue as previously flagged.🔧 Proposed change
- -# Modern Admin Theme — Developer & Designer Guide +# Modern Admin Theme — Developer & Designer Guide +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md` around lines 1 - 2, MD041 requires the top-level heading to be the first line; move the H1 "# Modern Admin Theme — Developer & Designer Guide" to line 1 by placing the image below it (or remove the leading image), ensuring the H1 string appears first, followed by a blank line and then the image Markdown so the header is the very first content in the file.htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md (1)
1-2:⚠️ Potential issue | 🟡 MinorMove the H1 to the first line to satisfy MD041.
This mirrors the prior review and still applies.🔧 Proposed change
- -# Modern Theme — Customization Guide +# Modern Theme — Customization Guide +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md` around lines 1 - 2, The Markdown H1 ("# Modern Theme — Customization Guide") is not on the very first line (an image markdown precedes it), triggering MD041; move the H1 to the first line of the document so the header is the first element (for example, swap the lines so "# Modern Theme — Customization Guide" is line 1 and the image markdown () follows), ensuring the H1 text remains exactly "Modern Theme — Customization Guide".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/themes/modern/modern.php`:
- Around line 100-102: Replace the direct filter_input usage with XMF's input
wrapper: use \Xmf\Request::getString('xoops_dark_mode', '0', 'COOKIE') to fetch
the cookie into $darkMode, then normalize its value to either '0' or '1' (e.g.,
cast/compare so anything truthy becomes '1' and default/invalid becomes '0')
before applying htmlspecialchars($darkMode, ENT_QUOTES | ENT_SUBSTITUTE,
'UTF-8'); ensure you update the assignment to $darkMode and keep the escaping
call intact.
---
Duplicate comments:
In `@htdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.md`:
- Around line 1-2: The Markdown H1 ("# Modern Theme — Customization Guide") is
not on the very first line (an image markdown precedes it), triggering MD041;
move the H1 to the first line of the document so the header is the first element
(for example, swap the lines so "# Modern Theme — Customization Guide" is line 1
and the image markdown () follows),
ensuring the H1 text remains exactly "Modern Theme — Customization Guide".
In `@htdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.md`:
- Around line 1-2: MD041 requires the top-level heading to be the first line;
move the H1 "# Modern Admin Theme — Developer & Designer Guide" to line 1 by
placing the image below it (or remove the leading image), ensuring the H1 string
appears first, followed by a blank line and then the image Markdown so the
header is the very first content in the file.
In `@htdocs/modules/system/themes/modern/README.md`:
- Around line 1-2: Move the H1 so it is the very first line of README.md to
satisfy markdownlint MD041: place the line "# XOOPS Modern Admin Theme" above
the image (currently ""), ensuring the H1
text remains exactly "XOOPS Modern Admin Theme" and the image follows
immediately after.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
htdocs/modules/system/themes/modern/README.mdhtdocs/modules/system/themes/modern/docs/CUSTOMIZATION_GUIDE.mdhtdocs/modules/system/themes/modern/docs/DEVELOPER_GUIDE.mdhtdocs/modules/system/themes/modern/docs/MODULE_INTEGRATION_GUIDE.mdhtdocs/modules/system/themes/modern/docs/USER_GUIDE.mdhtdocs/modules/system/themes/modern/modern.phphtdocs/modules/system/themes/modern/xotpl/xo_head.tpl
Comment on lines
+100
to
+102
| // $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode', FILTER_SANITIZE_STRING) ?: '0'; | ||
| $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode') ?: '0'; | ||
| $darkMode = htmlspecialchars($darkMode, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Xmf\Request class
fd -type f 'Request.php' | head -5Repository: XOOPS/XoopsCore25
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Find Xmf\Request class with correct fd syntax
fd -t f 'Request.php'Repository: XOOPS/XoopsCore25
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Examine the Xmf\Request class structure
cat -n htdocs/xoops_lib/vendor/xoops/xmf/src/Request.phpRepository: XOOPS/XoopsCore25
Length of output: 25275
🏁 Script executed:
#!/bin/bash
# View the file context around lines 100-102
sed -n '95,110p' htdocs/modules/system/themes/modern/modern.phpRepository: XOOPS/XoopsCore25
Length of output: 632
Replace filter_input() with Xmf\Request::getString() for guideline compliance.
filter_input() bypasses the required XMF input layer. Use \Xmf\Request::getString('xoops_dark_mode', '0', 'COOKIE') instead and normalize to '0'/'1' before escaping.
🔧 Proposed fix
- $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode') ?: '0';
- $darkMode = htmlspecialchars($darkMode, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
+ $darkMode = \Xmf\Request::getString('xoops_dark_mode', '0', 'COOKIE');
+ $darkMode = ($darkMode === '1') ? '1' : '0';
+ $darkMode = htmlspecialchars($darkMode, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/modern.php` around lines 100 - 102,
Replace the direct filter_input usage with XMF's input wrapper: use
\Xmf\Request::getString('xoops_dark_mode', '0', 'COOKIE') to fetch the cookie
into $darkMode, then normalize its value to either '0' or '1' (e.g.,
cast/compare so anything truthy becomes '1' and default/invalid becomes '0')
before applying htmlspecialchars($darkMode, ENT_QUOTES | ENT_SUBSTITUTE,
'UTF-8'); ensure you update the assignment to $darkMode and keep the escaping
call intact.
|
|
||
| // Get user preference for dark mode | ||
| $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode', FILTER_SANITIZE_STRING) ?: '0'; | ||
| // $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode', FILTER_SANITIZE_STRING) ?: '0'; |
There was a problem hiding this comment.
The commented out code should be removed rather than left in place. Keeping outdated or unused code as comments clutters the codebase and can be confusing for future maintainers. Since version control systems preserve the history, there's no need to keep this commented line.
Suggested change
| // $darkMode = filter_input(INPUT_COOKIE, 'xoops_dark_mode', FILTER_SANITIZE_STRING) ?: '0'; |
| * Override any CSS variable from modern.css :root to restyle the | ||
| * entire theme at once. Examples: | ||
| * | ||
| * Change the primary accent colour: |
There was a problem hiding this comment.
Inconsistent spelling: "colour" (British) should be "color" (American) to match the rest of the codebase and documentation. The word "color" is used consistently throughout CUSTOMIZATION_GUIDE.md and README.md.
Suggested change
| * Change the primary accent colour: | |
| * Change the primary accent color: |
|
|
||
| A practical guide for **site owners and theme designers** who want to customize the Modern admin theme's appearance without touching the theme's core files. | ||
|
|
||
| > **Developers** extending the theme's PHP or JavaScript behaviour should refer to [DEVELOPER_GUIDE.md](DEVELOPER_GUIDE.md) instead. |
There was a problem hiding this comment.
Inconsistent spelling: "behaviour" (British) should be "behavior" (American) to match the rest of the codebase. The American spelling "behavior" is used consistently in DEVELOPER_GUIDE.md and USER_GUIDE.md.
Suggested change
| > **Developers** extending the theme's PHP or JavaScript behaviour should refer to [DEVELOPER_GUIDE.md](DEVELOPER_GUIDE.md) instead. | |
| > **Developers** extending the theme's PHP or JavaScript behavior should refer to [DEVELOPER_GUIDE.md](DEVELOPER_GUIDE.md) instead. |
The include inside the loop intentionally re-reads different xoops_version.php config files per iteration; include_once would silently skip files already loaded elsewhere in the request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- custom.css: simplify header comment to avoid S125 (commented-out code) - xoops.css: change dark-mode footer text from #9ca3af to #d1d5db to meet WCAG AA 4.5:1 contrast minimum (~6.87:1 on #374151) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
htdocs/modules/system/themes/modern/css/xoops.css (4)
746-752: Globalfieldsetselector — same scoping concern as already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 746 - 752, The global "fieldset" CSS rule is too broad and may affect other themes/components; narrow its scope by replacing the global selector with a theme-specific or namespaced selector (e.g., a wrapper class such as ".modern fieldset", ".xoops-theme-modern fieldset" or a dedicated class like ".xo-fieldset") and update any markup that needs these styles to use that class; ensure the rule still uses the same declarations (border, border-radius, padding, margin, background) but only targets the namespaced selector to avoid leaking styles globally.
252-267: Global button selectors — same scoping concern as already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 252 - 267, The button rules use global selectors (input[type="submit"], input[type="button"], button.formButton, .formButton) which can unintentionally style unrelated elements; scope them by prefixing with the theme/form container class used across this theme (e.g., .theme-modern or .xoops or the page/form wrapper you already use) so only buttons inside that namespace get these styles, and consider renaming the generic .formButton class to a namespaced class (e.g., .theme-modern__formButton) to avoid collisions.
226-250: Unscoped form-element selectors — same concern as already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 226 - 250, The CSS form-element selectors (input[type="text"], input[type="password"], input[type="email"], input[type="number"], input[type="file"], select, textarea and their :focus rules) are unscoped and will affect all forms site-wide; scope them by prepending a theme/container selector (for example .modern-theme or a more specific wrapper like .xoops-form or .module-form) to both the base rule and the :focus rule so the styles only apply within that theme/container.
830-833: Hardcoded80pxcollapsed-sidebar magic number — same concern as already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 830 - 833, The CSS uses a hardcoded "80px" magic number in the rule for body.sidebar-open `#xo-logger-output`; replace this literal with a CSS custom property (e.g. var(--sidebar-collapsed-width)) so the collapsed-sidebar width is defined in one place and reused. Define --sidebar-collapsed-width alongside the sidebar's width declaration (or in :root) and update the selector body.sidebar-open `#xo-logger-output` to use var(--sidebar-collapsed-width) instead of 80px to keep styling consistent and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/themes/modern/css/xoops.css`:
- Around line 277-282: The global selectors input[type="radio"] and
input[type="checkbox"] should be scoped to the theme container like other rules;
replace or add the rule under the theme root selector used in this stylesheet
(the same container used for other scoped rules) so the CSS becomes
.<theme-container> input[type="radio"], .<theme-container>
input[type="checkbox"] { margin-right: 6px; cursor: pointer; } — locate the
existing global rule for input[type="radio"]/input[type="checkbox"] and move or
duplicate it under the theme container selector (keeping the same properties) to
avoid global leakage.
- Around line 649-653: The CSS currently exposes a global rule ".center {
text-align: center; margin: 20px auto }" which bleeds into all module admin
pages; restrict this by removing the unscoped ".center" selector and keep or
consolidate the styles under the scoped selector "#xo-modadmin-index .center"
(or rename the project-specific class to a unique name like ".xo-mod-center" and
update usages) so only elements inside "#xo-modadmin-index" receive the margin
and centering.
- Around line 34-107: The message classes (.errorMsg, .warningMsg, .resultMsg,
.confirmMsg) use hardcoded hex values for background and color (and their
body.dark-mode overrides) while only border-left-color uses design tokens;
replace those hex literals by new CSS variables (e.g. --danger-bg,
--danger-text, --danger-bg-dark, --danger-text-dark and analogous --warning-*,
--success-*, --info-*), add those token definitions to :root (and dark-mode
overrides) in modern.css, and then reference these variables in the existing
selectors so backgrounds/text colors follow the token system consistently.
- Around line 241-250: The current focus rule (input[type="text"]:focus,
input[type="password"]:focus, input[type="email"]:focus,
input[type="number"]:focus, select:focus, textarea:focus) removes the native
outline and applies a 10% blue box-shadow that fails WCAG 1.4.11 and disappears
in dark mode; restore or replace it with an accessible focus indicator (e.g.,
remove outline: none or replace it with a visible ring using a CSS variable like
--focus-ring with sufficient opacity to meet 3:1 contrast) and add a dark-mode
override (body.dark-mode input[type="text"]:focus, etc.) that sets a contrasting
focus color/opacity (or a different color) so the focus ring remains visible
against `#374151`; ensure the new box-shadow/outline meets 3:1 contrast on both
light and dark backgrounds and apply the same change to the select and textarea
focus selectors.
---
Duplicate comments:
In `@htdocs/modules/system/themes/modern/css/xoops.css`:
- Around line 746-752: The global "fieldset" CSS rule is too broad and may
affect other themes/components; narrow its scope by replacing the global
selector with a theme-specific or namespaced selector (e.g., a wrapper class
such as ".modern fieldset", ".xoops-theme-modern fieldset" or a dedicated class
like ".xo-fieldset") and update any markup that needs these styles to use that
class; ensure the rule still uses the same declarations (border, border-radius,
padding, margin, background) but only targets the namespaced selector to avoid
leaking styles globally.
- Around line 252-267: The button rules use global selectors
(input[type="submit"], input[type="button"], button.formButton, .formButton)
which can unintentionally style unrelated elements; scope them by prefixing with
the theme/form container class used across this theme (e.g., .theme-modern or
.xoops or the page/form wrapper you already use) so only buttons inside that
namespace get these styles, and consider renaming the generic .formButton class
to a namespaced class (e.g., .theme-modern__formButton) to avoid collisions.
- Around line 226-250: The CSS form-element selectors (input[type="text"],
input[type="password"], input[type="email"], input[type="number"],
input[type="file"], select, textarea and their :focus rules) are unscoped and
will affect all forms site-wide; scope them by prepending a theme/container
selector (for example .modern-theme or a more specific wrapper like .xoops-form
or .module-form) to both the base rule and the :focus rule so the styles only
apply within that theme/container.
- Around line 830-833: The CSS uses a hardcoded "80px" magic number in the rule
for body.sidebar-open `#xo-logger-output`; replace this literal with a CSS custom
property (e.g. var(--sidebar-collapsed-width)) so the collapsed-sidebar width is
defined in one place and reused. Define --sidebar-collapsed-width alongside the
sidebar's width declaration (or in :root) and update the selector
body.sidebar-open `#xo-logger-output` to use var(--sidebar-collapsed-width)
instead of 80px to keep styling consistent and maintainable.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
htdocs/modules/system/themes/modern/css/custom.csshtdocs/modules/system/themes/modern/css/xoops.csshtdocs/modules/system/themes/modern/menu.php
Comment on lines
+34
to
+107
| .errorMsg, | ||
| .warningMsg, | ||
| .resultMsg, | ||
| .confirmMsg { | ||
| padding: 16px 20px; | ||
| border-radius: var(--radius); | ||
| margin-bottom: 20px; | ||
| font-size: 14px; | ||
| line-height: 1.6; | ||
| border-left: 4px solid; | ||
| } | ||
|
|
||
| .errorMsg { | ||
| background: #fef2f2; | ||
| border-left-color: var(--danger); | ||
| color: #991b1b; | ||
| } | ||
|
|
||
| .warningMsg { | ||
| background: #fffbeb; | ||
| border-left-color: var(--warning); | ||
| color: #92400e; | ||
| } | ||
|
|
||
| .resultMsg { | ||
| background: #f0fdf4; | ||
| border-left-color: var(--success); | ||
| color: #166534; | ||
| } | ||
|
|
||
| .confirmMsg { | ||
| background: #eff6ff; | ||
| border-left-color: var(--info); | ||
| color: #1e40af; | ||
| } | ||
|
|
||
| .errorMsg strong, | ||
| .warningMsg strong, | ||
| .resultMsg strong, | ||
| .confirmMsg strong { | ||
| display: block; | ||
| margin-bottom: 4px; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .confirmMsg input[type="submit"], | ||
| .confirmMsg input[type="button"] { | ||
| margin-top: 8px; | ||
| margin-right: 8px; | ||
| } | ||
|
|
||
| body.dark-mode .errorMsg { | ||
| background: #450a0a; | ||
| color: #fca5a5; | ||
| border-left-color: #f87171; | ||
| } | ||
|
|
||
| body.dark-mode .warningMsg { | ||
| background: #451a03; | ||
| color: #fcd34d; | ||
| border-left-color: #fbbf24; | ||
| } | ||
|
|
||
| body.dark-mode .resultMsg { | ||
| background: #052e16; | ||
| color: #86efac; | ||
| border-left-color: #34d399; | ||
| } | ||
|
|
||
| body.dark-mode .confirmMsg { | ||
| background: #172554; | ||
| color: #93c5fd; | ||
| border-left-color: #60a5fa; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded hex colors in message classes are inconsistent with the design-token approach used everywhere else in this file.
border-left-color correctly uses var(--danger), var(--warning), var(--success), and var(--info), but the background and color properties fall back to magic hex literals — and their dark-mode overrides at lines 85–107 do the same. This defeats the purpose of the token system: a downstream custom.css override of --danger (for example) will correctly recolour the left border but leave the background and text at their original hues.
Consider extending the design-token set and using them consistently:
♻️ Proposed refactor (extend tokens in modern.css :root, then reference here)
-/* In modern.css :root, add: */
+:root {
+ --error-bg: `#fef2f2`;
+ --error-text: `#991b1b`;
+ --warning-bg: `#fffbeb`;
+ --warning-text: `#92400e`;
+ --success-bg: `#f0fdf4`;
+ --success-text: `#166534`;
+ --info-bg: `#eff6ff`;
+ --info-text: `#1e40af`;
+ /* dark overrides live in dark.css */
+}
.errorMsg {
- background: `#fef2f2`;
+ background: var(--error-bg);
border-left-color: var(--danger);
- color: `#991b1b`;
+ color: var(--error-text);
}
/* … same pattern for .warningMsg, .resultMsg, .confirmMsg … */
-body.dark-mode .errorMsg {
- background: `#450a0a`;
- color: `#fca5a5`;
- border-left-color: `#f87171`;
-}
+/* Move dark-mode overrides to dark.css: */
+body.dark-mode .errorMsg { /* or :root[class~="dark-mode"] */
+ --error-bg: `#450a0a`;
+ --error-text: `#fca5a5`;
+ --danger: `#f87171`;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 34 - 107, The
message classes (.errorMsg, .warningMsg, .resultMsg, .confirmMsg) use hardcoded
hex values for background and color (and their body.dark-mode overrides) while
only border-left-color uses design tokens; replace those hex literals by new CSS
variables (e.g. --danger-bg, --danger-text, --danger-bg-dark, --danger-text-dark
and analogous --warning-*, --success-*, --info-*), add those token definitions
to :root (and dark-mode overrides) in modern.css, and then reference these
variables in the existing selectors so backgrounds/text colors follow the token
system consistently.
Comment on lines
+241
to
+250
| input[type="text"]:focus, | ||
| input[type="password"]:focus, | ||
| input[type="email"]:focus, | ||
| input[type="number"]:focus, | ||
| select:focus, | ||
| textarea:focus { | ||
| outline: none; | ||
| border-color: var(--primary); | ||
| box-shadow: 0 0 0 3px rgba(37, 99, 235, 0.1); | ||
| } |
There was a problem hiding this comment.
outline: none with a 10 % opacity replacement shadow fails WCAG SC 1.4.11 Non-text Contrast, and dark mode has no :focus override.
Two concrete issues:
-
Insufficient contrast: The replacement indicator is
box-shadow: 0 0 0 3px rgba(37, 99, 235, 0.1). At 10 % opacity, this renders near-invisible against a white background and completely invisible against the dark-mode background (#374151). Default browser focus indicators are exempt from contrast requirements under WCAG 2.1 AA — but once you override those styles, you are responsible for ensuring a minimum contrast ratio of 3:1. -
Missing dark-mode
:focusrule: Lines 435–445 overridebackground,border-color, andcolorfor dark-mode inputs, but there is nobody.dark-mode input[type="text"]:focus(etc.) rule, so the 10 % blue shadow persists unchanged over a#374151dark input — essentially invisible.
Avoid removing focus indicator outlines via outline: none unless a more accessible alternative is applied.
🛡️ Proposed fix
input[type="text"]:focus,
input[type="password"]:focus,
input[type="email"]:focus,
input[type="number"]:focus,
select:focus,
textarea:focus {
- outline: none;
+ outline: 2px solid var(--primary);
+ outline-offset: 2px;
border-color: var(--primary);
- box-shadow: 0 0 0 3px rgba(37, 99, 235, 0.1);
+ box-shadow: 0 0 0 3px rgba(37, 99, 235, 0.2);
}Add to the dark-mode block (after line 445):
+body.dark-mode input[type="text"]:focus,
+body.dark-mode input[type="password"]:focus,
+body.dark-mode input[type="email"]:focus,
+body.dark-mode input[type="number"]:focus,
+body.dark-mode select:focus,
+body.dark-mode textarea:focus {
+ outline-color: `#60a5fa`;
+ border-color: `#60a5fa`;
+ box-shadow: 0 0 0 3px rgba(96, 165, 250, 0.3);
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 241 - 250,
The current focus rule (input[type="text"]:focus, input[type="password"]:focus,
input[type="email"]:focus, input[type="number"]:focus, select:focus,
textarea:focus) removes the native outline and applies a 10% blue box-shadow
that fails WCAG 1.4.11 and disappears in dark mode; restore or replace it with
an accessible focus indicator (e.g., remove outline: none or replace it with a
visible ring using a CSS variable like --focus-ring with sufficient opacity to
meet 3:1 contrast) and add a dark-mode override (body.dark-mode
input[type="text"]:focus, etc.) that sets a contrasting focus color/opacity (or
a different color) so the focus ring remains visible against `#374151`; ensure the
new box-shadow/outline meets 3:1 contrast on both light and dark backgrounds and
apply the same change to the select and textarea focus selectors.
Comment on lines
+277
to
+282
| /* Radio and checkboxes */ | ||
| input[type="radio"], | ||
| input[type="checkbox"] { | ||
| margin-right: 6px; | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Global input[type="radio"] and input[type="checkbox"] selectors — same scoping concern as the form inputs flagged above.
The properties are minimal (margin-right, cursor), but for consistency with the broader scoping recommendation, these should also be prefixed with the theme container.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 277 - 282,
The global selectors input[type="radio"] and input[type="checkbox"] should be
scoped to the theme container like other rules; replace or add the rule under
the theme root selector used in this stylesheet (the same container used for
other scoped rules) so the CSS becomes .<theme-container> input[type="radio"],
.<theme-container> input[type="checkbox"] { margin-right: 6px; cursor: pointer;
} — locate the existing global rule for
input[type="radio"]/input[type="checkbox"] and move or duplicate it under the
theme container selector (keeping the same properties) to avoid global leakage.
Comment on lines
+649
to
+653
| #xo-modadmin-index .center, | ||
| .center { | ||
| text-align: center; | ||
| margin: 20px auto; | ||
| } |
There was a problem hiding this comment.
Unscoped .center rule silently bleeds into all module admin content.
The second selector on line 651, .center { ... }, matches every element with that class across all module admin pages — not just #xo-modadmin-index. In particular, margin: 20px auto applied globally can collapse or mis-center elements in arbitrary module content that happens to use the very common .center class.
♻️ Proposed fix — remove the unscoped variant
-#xo-modadmin-index .center,
-.center {
+#xo-modadmin-index .center {
text-align: center;
margin: 20px auto;
}📝 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
| #xo-modadmin-index .center, | |
| .center { | |
| text-align: center; | |
| margin: 20px auto; | |
| } | |
| `#xo-modadmin-index` .center { | |
| text-align: center; | |
| margin: 20px auto; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/themes/modern/css/xoops.css` around lines 649 - 653,
The CSS currently exposes a global rule ".center { text-align: center; margin:
20px auto }" which bleeds into all module admin pages; restrict this by removing
the unscoped ".center" selector and keep or consolidate the styles under the
scoped selector "#xo-modadmin-index .center" (or rename the project-specific
class to a unique name like ".xo-mod-center" and update usages) so only elements
inside "#xo-modadmin-index" receive the margin and centering.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1622 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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 by CodeRabbit
New Features
Documentation
Bug Fixes