Refactor: Staff payroll selected staff page UI improvements#20830
Open
RohanSGX wants to merge 2 commits into
Open
Refactor: Staff payroll selected staff page UI improvements#20830RohanSGX wants to merge 2 commits into
RohanSGX wants to merge 2 commits into
Conversation
Contributor
WalkthroughThe ChangesHR Staff Salary Page Styling Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
RohanSGX
added
UI
…selected staff table
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/main/webapp/hr/hr_staff_salary_1.xhtml (6)
298-300: 💤 Low valueWrap row index in
h:outputTextfor consistency.Other columns use
h:outputTextfor values; this plain EL expression is inconsistent.Suggested fix
<p:column> - #{i+1} + <h:outputText value="#{i+1}" /> </p:column>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` around lines 298 - 300, Replace the bare EL expression "#{i+1}" inside the p:column with an h:outputText component for consistency with other columns; locate the p:column that currently contains the plain "#{i+1}" (the row index) and change it to use h:outputText with value="#{i+1}" so the column follows the same rendering pattern as other cells.
364-384: ⚡ Quick winUse
h:outputTextfor report header labels.The static text in
<span>elements and inline prefixes should useh:outputTextfor consistency with ERP page patterns.Suggested fix
<div class="report-header-wrap"> - <span class="report-title-label">Staff payroll — selected staff</span> + <span class="report-title-label"><h:outputText value="Staff payroll — selected staff" /></span> <span class="report-cycle-label"> <h:outputText value="#{salaryCycleController.current.name}" /> </span> <h:panelGroup rendered="#{salaryCycleController.reportKeyWord.institution ne null}"> <p class="report-meta"> <h:outputText value="#{salaryCycleController.reportKeyWord.institution.name}" /> </p> </h:panelGroup> <h:panelGroup rendered="#{salaryCycleController.reportKeyWord.department ne null}"> <p class="report-meta"> - Department: <h:outputText value="#{salaryCycleController.reportKeyWord.department.name}" /> + <h:outputText value="Department: " /><h:outputText value="#{salaryCycleController.reportKeyWord.department.name}" /> </p> </h:panelGroup> <h:panelGroup rendered="#{salaryCycleController.reportKeyWord.roster ne null}"> <p class="report-meta"> - Roster: <h:outputText value="#{salaryCycleController.reportKeyWord.roster.name}" /> + <h:outputText value="Roster: " /><h:outputText value="#{salaryCycleController.reportKeyWord.roster.name}" /> </p> </h:panelGroup> </div>As per coding guidelines: "Use
h:outputTextfor headings and labels in ERP pages instead of plain text"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` around lines 364 - 384, Replace plain static text used for headings/labels with h:outputText components: change the "Staff payroll — selected staff" inside the span.report-title-label to an <h:outputText> value, replace the report-cycle-label static wrapper if needed, and convert inline prefixes "Department:" and "Roster:" inside the <p class="report-meta"> blocks to <h:outputText> components; update the markup around those spans/phrases so they use h:outputText while keeping existing references like salaryCycleController.current.name and salaryCycleController.reportKeyWord.department/roster.name unchanged.
211-211: 💤 Low valueRedundant
.greenTextclass shadows global definition.This class is already defined in
maincss.css(line 670). The duplicate definition with!importantmay cause unintended style conflicts or make future maintenance harder. Consider removing this line and relying on the global stylesheet.Suggested fix
- .greenText { color: `#2e7d32` !important; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` at line 211, Remove the duplicate .greenText CSS rule in hr_staff_salary_1.xhtml (the ".greenText { color: `#2e7d32` !important; }" snippet) and rely on the global definition in maincss.css instead; if the !important was added to override specificity, move that change into the global stylesheet (maincss.css) or adjust selectors rather than keeping a redundant inline rule in hr_staff_salary_1.xhtml.
403-405: ⚡ Quick winWrap dynamic header text in
h:outputText.The dynamic column headers from
ui:repeatuse plain EL expressions. Wrap them inh:outputTextfor consistency and proper escaping.Suggested fix
<ui:repeat value="#{salaryCycleController.headersAdd}" var="h"> - <th>#{h}</th> + <th><h:outputText value="#{h}" /></th> </ui:repeat><ui:repeat value="#{salaryCycleController.headersSub}" var="hh"> - <th>#{hh}</th> + <th><h:outputText value="#{hh}" /></th> </ui:repeat>Also applies to: 411-413
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` around lines 403 - 405, The ui:repeat rendering dynamic headers uses raw EL (ui:repeat value="#{salaryCycleController.headersAdd}" var="h") and should wrap each header expression in h:outputText to ensure proper escaping and consistent rendering; replace the inline #{h} usage with an h:outputText component (e.g., use h:outputText value="#{h}") for the repeat at the current block and apply the same change to the second occurrence mentioned (the block around lines 411-413).
491-493: 💤 Low valueUse
h:outputTextinstead ofp:outputLabelfor displaying formatted datetime.
p:outputLabelis semantically intended for labeling form inputs, not for displaying formatted values. Useh:outputTextwith the converter for proper semantic markup.Suggested fix
- <p:outputLabel value="#{commonFunctionsProxy.currentDateTime}"> + <h:outputText value="#{commonFunctionsProxy.currentDateTime}"> <f:convertDateTime pattern="yyyy MMMM dd hh:mm:ss a" /> - </p:outputLabel> + </h:outputText>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` around lines 491 - 493, Replace the p:outputLabel used to display the formatted datetime with h:outputText because p:outputLabel is for labeling inputs; change the component from p:outputLabel to h:outputText while keeping the f:convertDateTime converter and its pattern (reference p:outputLabel and f:convertDateTime in the snippet) so the same value #{commonFunctionsProxy.currentDateTime} is rendered semantically as output text rather than a form label.
214-217: ⚡ Quick winUse
h:outputTextfor page headings per coding guidelines.Plain text in
<p>tags should be wrapped inh:outputTextfor consistency with ERP page patterns.Suggested fix
<div class="page-header noPrintButton"> - <p class="page-title">Staff payroll — selected staff</p> - <p class="page-subtitle">Select staff from the list then process payroll in the Pay role tab</p> + <p class="page-title"><h:outputText value="Staff payroll — selected staff" /></p> + <p class="page-subtitle"><h:outputText value="Select staff from the list then process payroll in the Pay role tab" /></p> </div>As per coding guidelines: "Use
h:outputTextfor headings and labels in ERP pages instead of plain text"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/webapp/hr/hr_staff_salary_1.xhtml` around lines 214 - 217, Replace the raw text inside the page header <p> elements with JSF h:outputText components so headings follow ERP patterns: locate the <div class="page-header noPrintButton"> and update the <p class="page-title"> and <p class="page-subtitle"> elements to render their literal strings via h:outputText (preserving the same CSS classes and text content) instead of plain text nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/webapp/hr/hr_staff_salary_1.xhtml`:
- Around line 298-300: Replace the bare EL expression "#{i+1}" inside the
p:column with an h:outputText component for consistency with other columns;
locate the p:column that currently contains the plain "#{i+1}" (the row index)
and change it to use h:outputText with value="#{i+1}" so the column follows the
same rendering pattern as other cells.
- Around line 364-384: Replace plain static text used for headings/labels with
h:outputText components: change the "Staff payroll — selected staff" inside the
span.report-title-label to an <h:outputText> value, replace the
report-cycle-label static wrapper if needed, and convert inline prefixes
"Department:" and "Roster:" inside the <p class="report-meta"> blocks to
<h:outputText> components; update the markup around those spans/phrases so they
use h:outputText while keeping existing references like
salaryCycleController.current.name and
salaryCycleController.reportKeyWord.department/roster.name unchanged.
- Line 211: Remove the duplicate .greenText CSS rule in hr_staff_salary_1.xhtml
(the ".greenText { color: `#2e7d32` !important; }" snippet) and rely on the global
definition in maincss.css instead; if the !important was added to override
specificity, move that change into the global stylesheet (maincss.css) or adjust
selectors rather than keeping a redundant inline rule in
hr_staff_salary_1.xhtml.
- Around line 403-405: The ui:repeat rendering dynamic headers uses raw EL
(ui:repeat value="#{salaryCycleController.headersAdd}" var="h") and should wrap
each header expression in h:outputText to ensure proper escaping and consistent
rendering; replace the inline #{h} usage with an h:outputText component (e.g.,
use h:outputText value="#{h}") for the repeat at the current block and apply the
same change to the second occurrence mentioned (the block around lines 411-413).
- Around line 491-493: Replace the p:outputLabel used to display the formatted
datetime with h:outputText because p:outputLabel is for labeling inputs; change
the component from p:outputLabel to h:outputText while keeping the
f:convertDateTime converter and its pattern (reference p:outputLabel and
f:convertDateTime in the snippet) so the same value
#{commonFunctionsProxy.currentDateTime} is rendered semantically as output text
rather than a form label.
- Around line 214-217: Replace the raw text inside the page header <p> elements
with JSF h:outputText components so headings follow ERP patterns: locate the
<div class="page-header noPrintButton"> and update the <p class="page-title">
and <p class="page-subtitle"> elements to render their literal strings via
h:outputText (preserving the same CSS classes and text content) instead of plain
text nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a76742de-8882-4ce8-b1d7-3ace87839698
📒 Files selected for processing (1)
src/main/webapp/hr/hr_staff_salary_1.xhtml
buddhika75
requested changes
May 30, 2026
|
|
||
| <h:form id="print" styleClass="salary-summary-form"> | ||
|
|
||
| <h:outputStylesheet> |
Member
There was a problem hiding this comment.
We do nto usually use inline / page specific css. We need to add to a css file or use existing css and call the css classes.
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
2 participants
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
UI refactor of the staff payroll selected staff page, consistent with the salary summary and payroll pages styling and layout pattern.
Changes
h:panelGrid columns="2"filter fields in the Staff list tab with a 2-columnfields-gridlayout using stacked label + field groupstab-contentflex column with gap for both tabstab-actionsrows in both tabsstaff-tablestyling top:dataTable— muted uppercase headers, consistent padding, hover highlightStaff List→Staff list,Pay Role→Pay role) and column headersborder: 1px solid blacktable styling with cleanpayroll-tablestyles matching reference pagesstyleattributes fromtdandthelements — alignment handled viacol-leftandcol-nameclassesh:outputLabel/h:outputTextusage in data cells with consistenth:outputTextreport-table-footerdiv with flex layout matching reference pagesProcess Hold List→Process hold list,Fill All Staff→Fill all staff)<br/>spacers throughoutstaff_payroll.xls→staff_payroll_selected.xlsappendTo="@body"andautoWidth="false"onp:selectOneMenuto prevent overlay closing immediately when wrapped insidep:tabViewNo functional changes
All bean bindings, action methods,
ui:repeatdynamic columns, staff selection logic, Excel export, and print behaviour are unchanged.Summary by CodeRabbit