fix Admin help toggle #1553
Merged
fix Admin help toggle #1553
Conversation
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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the Admin “help” toggle by swapping the hide icon to a grayscale version, enforcing inline display, and improving the show/hide logic in JS, plus removes an extra character in the banners template.
- Swap help-hide icon to
help_bw.pngand adddisplay: inline;across theme and template files - Change JS to explicitly hide/show each toggle link and initially hide the “hide” icon
- Remove stray
>from the banner count check insystem_banners.tpl
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/modules/system/themes/dark/modules/system/admin/system_header.tpl | Added display: inline; style and swapped to help_bw.png |
| htdocs/modules/system/templates/admin/system_header.tpl | Added display: inline; style and swapped to help_bw.png |
| htdocs/modules/system/templates/admin/system_header.html | Added display: inline; style and swapped to help_bw.png |
| htdocs/modules/system/templates/admin/system_banners.tpl | Removed extra > from the banner count condition |
| htdocs/modules/system/js/admin.js | Replaced .toggle() calls with explicit .hide()/ .show(), plus initial state setup |
Comments suppressed due to low confidence (1)
htdocs/modules/system/js/admin.js:33
- [nitpick] Consider using
.on('click', ...)instead of.click(...)for more consistent event binding and future compatibility.
$("a.help_view").click(function () {
htdocs/modules/system/js/admin.js
Outdated
Comment on lines
31
to
42
| $("a.help_hide").hide(); | ||
|
|
||
| $("a.help_view").click(function () { | ||
| $("div#xo-system-help").slideToggle(1000); | ||
| $("a.help_view").toggle(); | ||
| $("a.help_hide").toggle(); | ||
| $("a.help_view").hide(); | ||
| $("a.help_hide").show(); | ||
| }); | ||
|
|
||
| $("a.help_hide").click(function () { | ||
| $("div#xo-system-help").slideToggle(1000); | ||
| $("a.help_view").toggle(); | ||
| $("a.help_hide").toggle(); | ||
| $("a.help_hide").hide(); | ||
| $("a.help_view").show(); |
There was a problem hiding this comment.
[nitpick] You may want to cache these jQuery selections (e.g., const $view = $("a.help_view");) to avoid querying the DOM multiple times for the same elements.
| alt="<{$smarty.const._AM_SYSTEM_HELP_VIEW}>"/></a> | ||
| <a class="cursorhelp tooltip help_hide" title="<{$smarty.const._AM_SYSTEM_HELP_HIDE}>" | ||
| style="background-image:url('<{$theme_img}>/bc_separator_end.png');"><img src="<{xoAdminIcons 'help.png'}>" | ||
| style="background-image:url('<{$theme_img}>/bc_separator_end.png'); display: inline;"><img src="<{xoAdminIcons 'help_bw.png'}>" |
There was a problem hiding this comment.
[nitpick] Avoid inline styles in templates; consider moving display: inline; into a shared CSS class to reduce repetition and improve maintainability.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the admin help toggle functionality by updating the UI markup, JavaScript behavior, and CSS styles across several templates and assets.
- Updated the HTML templates to use new classes (e.g., xo-help-button) for toggle behavior and removed redundant inline styles.
- Improved the JavaScript implementation by caching selectors and using delegated event handling with appropriate class manipulation.
- Fixed a syntax error in the banners template by removing an extraneous character.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/modules/system/themes/dark/modules/system/admin/system_header.tpl | Updated markup for help toggle with new CSS classes. |
| htdocs/modules/system/templates/admin/system_header.tpl | Similar markup adjustments for a consistent toggle behavior. |
| htdocs/modules/system/templates/admin/system_header.html | Updated help_hide element ensuring display style consistency. |
| htdocs/modules/system/templates/admin/system_banners.tpl | Removed an extraneous closing bracket causing a syntax error. |
| htdocs/modules/system/js/admin.js | Refactored event handling for help toggle with cached selectors. |
| htdocs/modules/system/css/button.css | Added CSS for the new xo-help-button and its hidden state. |
Comments suppressed due to low confidence (1)
htdocs/modules/system/templates/admin/system_banners.tpl:4
- The condition expression contains an extraneous '>' at the end, which could cause a syntax error. Please remove the extra character to ensure proper template syntax.
<{if isset($banner_count) && $banner_count == true}>}>
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.
No description provided.