refactor(tooltips): standardize popup template names and update refer…#1206
refactor(tooltips): standardize popup template names and update refer…#1206JohnVillalovos wants to merge 1 commit intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes naming for AJAX popup Smarty templates (reservation/resource/user) and updates the corresponding PHP page classes to render the renamed templates, aiming for more consistent popup/template naming across the tooltip/popup system.
Changes:
- Renamed AJAX popup templates to
*_popup.tpl(reservation/resource/user) and removed the legacy filenames. - Updated
Pages/Ajax/*page classes to render the renamed templates. - Minor formatting/cleanup within
tpl/Ajax/resource_popup.tpl.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl/Ajax/user_popup.tpl | New user details popup template (replacement for user_details.tpl). |
| tpl/Ajax/user_details.tpl | Removed legacy user details popup template. |
| tpl/Ajax/reservation_popup.tpl | New reservation tooltip template (replacement for respopup.tpl). |
| tpl/Ajax/respopup.tpl | Removed legacy reservation tooltip template. |
| tpl/Ajax/resource_popup.tpl | Minor formatting adjustments within the resource popup template. |
| Pages/Ajax/UserDetailsPopupPage.php | Updated to display Ajax/user_popup.tpl. |
| Pages/Ajax/ResourceDetailsPage.php | Updated to display Ajax/resource_popup.tpl. |
| Pages/Ajax/ReservationPopupPage.php | Updated to display Ajax/reservation_popup.tpl. |
…ences Template changes: tpl/Ajax/respopup.tpl → tpl/Ajax/reservation_popup.tpl tpl/Ajax/resourcedetails.tpl → tpl/Ajax/resource_popup.tpl tpl/Ajax/user_details.tpl → tpl/Ajax/user_popup.tpl Updated references: Adjusted the corresponding PHP pages to use the new template names: Pages/Ajax/ReservationPopupPage.php Pages/Ajax/ResourceDetailsPage.php Pages/Ajax/UserDetailsPopupPage.php These changes ensure more consistent and predictable naming between templates and their associated JS.
f23c840 to
3a50231
Compare
| {if isset($readonly) && $readonly && isset($tooltip) && $tooltip} | ||
| <span class="customAttribute readonly fw-bold">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | ||
| <i class="bi bi-asterisk text-danger align-top text-small"></i> | ||
| {/if}</span> | ||
| {else} | ||
| <label | ||
| class="customAttribute {if isset($readonly) && $readonly}readonly{elseif isset($searchmode) && $searchmode}search{else}standard{/if} fw-bold" | ||
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | ||
| <i class="bi bi-asterisk text-danger align-top text-small"></i> | ||
| {/if}</label> |
There was a problem hiding this comment.
In readonly mode these templates render no form control with id $attributeId, but the else branch still emits a <label for="$attributeId">... when tooltip is not set. This produces invalid HTML and breaks label-to-control accessibility. Consider removing the for attribute (or using a non-label element) whenever $readonly is true, not only when $tooltip is enabled.
| {/if}</span> | ||
| {else} | ||
| <label class="customAttribute {if $readonly}readonly{elseif $searchmode}search{else}standard{/if} fw-bold" | ||
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} |
There was a problem hiding this comment.
Readonly rendering outputs only a <span> for the value, so there is no element with id $attributeId to associate the <label for="$attributeId"> with. Because the template only avoids <label> when tooltip is set, you still get invalid label associations in other readonly contexts. Consider using <span>/<div> (or removing for) whenever $readonly is true.
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | |
| {if !$readonly}for="{$attributeId}"{/if}>{$attribute->Label()}{if $attribute->Required() && !$searchmode} |
| {/if}</span> | ||
| {else} | ||
| <label class="customAttribute d-block {if $readonly}readonly{elseif $searchmode}search{else}standard{/if} fw-bold" | ||
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} |
There was a problem hiding this comment.
In readonly mode there is no input/select with id $attributeId, but the template still emits a <label for="$attributeId"> unless tooltip is set. This results in invalid HTML and incorrect accessibility semantics. Consider removing the for attribute (or using a non-label element) for all readonly renders.
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | |
| {if !$readonly}for="{$attributeId}"{/if}>{$attribute->Label()}{if $attribute->Required() && !$searchmode} |
| {if $hideUserInfo || $hideDetails} | ||
| {translate key=Private} | ||
| {else} | ||
| {$fullName} |
There was a problem hiding this comment.
This popup HTML is injected into a Bootstrap tooltip with data-bs-html=true (see reservationPopup.js). $fullName is output without HTML escaping, so any </& content would be interpreted as markup inside the tooltip. Please escape $fullName before output.
| {$fullName} | |
| {$fullName|escape} |
| <div class="resources"> | ||
| {translate key="Resources"} ({$resources|@count}): | ||
| {foreach from=$resources item=resource name=resource_loop} | ||
| {$resource->Name()} |
There was a problem hiding this comment.
Resource names are inserted into HTML tooltip content via {$resource->Name()} without escaping. Since tooltip rendering uses data-bs-html=true, resource names containing special characters could be interpreted as markup. Please apply HTML escaping to Name() output here.
| {$resource->Name()} | |
| {$resource->Name()|escape:'html'} |
| <div class="accessories"> | ||
| {translate key="Accessories"} ({$accessories|@count}): | ||
| {foreach from=$accessories item=accessory name=accessory_loop} | ||
| {$accessory->Name} ({$accessory->QuantityReserved}) |
There was a problem hiding this comment.
Accessory names are output unescaped inside HTML tooltip content (data-bs-html=true). Please HTML-escape $accessory->Name to avoid markup injection/broken tooltip rendering.
| {$accessory->Name} ({$accessory->QuantityReserved}) | |
| {$accessory->Name|escape:'html'} ({$accessory->QuantityReserved}) |
| {if $readonly && isset($tooltip) && $tooltip} | ||
| <span class="customAttribute readonly fw-bold">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | ||
| <i class="bi bi-asterisk text-danger align-top text-small"></i> | ||
| {/if}</span> | ||
| {else} | ||
| <label class="customAttribute {if $readonly}readonly{elseif $searchmode}search{else}standard{/if} fw-bold" | ||
| for="{$attributeId}">{$attribute->Label()}{if $attribute->Required() && !$searchmode} | ||
| <i class="bi bi-asterisk text-danger align-top text-small"></i> | ||
| {/if}</label> |
There was a problem hiding this comment.
When $readonly is true the control renders a <span> value (no matching element with id $attributeId), but the template still outputs <label for="$attributeId">... unless tooltip is set. This is invalid markup and harms accessibility; switch to a non-label element or drop the for attribute for all readonly renders.
| {capture "email"} | ||
| <div class="email"> | ||
| {if !$hideUserInfo && !$hideDetails} | ||
| {$email} |
There was a problem hiding this comment.
Because this template is rendered as HTML inside a Bootstrap tooltip (data-bs-html=true), $email is treated as markup. It's currently output without escaping; please HTML-escape it (or output a deliberately constructed safe link) to prevent injection.
| {$email} | |
| {$email|escape:'html'} |
| {capture "phone"} | ||
| <div class="phone"> | ||
| {if !$hideUserInfo && !$hideDetails} | ||
| {$phone} |
There was a problem hiding this comment.
$phone is output unescaped inside HTML tooltip content (data-bs-html=true). Please HTML-escape it (or generate a safe tel: link) to avoid potential markup injection/broken rendering.
| {$phone} | |
| {$phone|escape:'html'} |
…ences
Template changes:
tpl/Ajax/respopup.tpl → tpl/Ajax/reservation_popup.tpl tpl/Ajax/resourcedetails.tpl → tpl/Ajax/resource_popup.tpl tpl/Ajax/user_details.tpl → tpl/Ajax/user_popup.tpl
Updated references:
Adjusted the corresponding PHP pages to use the new template names:
Pages/Ajax/ReservationPopupPage.php
Pages/Ajax/ResourceDetailsPage.php
Pages/Ajax/UserDetailsPopupPage.php
These changes ensure more consistent and predictable naming between templates and their associated JS.