refactor represantativesPicker and styles improves#23
refactor represantativesPicker and styles improves#23MEDOYED wants to merge 3 commits intomkaminetskyi:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the representatives picker component to receive representatives data directly from the controller instead of using a boolean flag, and implements various style improvements to the navigation and desktop topbar components.
Changes:
- Refactored representatives picker to accept representatives list directly rather than using
renderRepresentativesPickerboolean flag - Fixed navigation active state identifier from
accounts-overdue-debtstooverdue-debts - Updated navigation CSS with adjusted spacing (gaps reduced, margin removed)
- Enhanced desktop topbar logo styling with background, border, and improved layout
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/com/demo/demotaskforfeside/controller/AccountsOverdueDebtsController.java |
Added Model parameter and hardcoded test representatives list and userRole to pass to the view |
src/main/resources/templates/pages/overdue-debts/overdue-debts.html |
Removed renderRepresentativesPicker parameter, added representatives parameter, and updated template structure to use inline content wrapper |
src/main/resources/templates/fragments/navigation/navigation.html |
Fixed active navigation identifier from accounts-overdue-debts to overdue-debts |
src/main/resources/templates/fragments/navigation/navigation.css |
Adjusted spacing with reduced gaps (18px→15px, 10px→5px) and removed top margin from section titles |
src/main/resources/templates/fragments/general-one-s-report-page-content/general-one-s-report-page-content.html |
Moved representatives picker into toolbar grid, changed from boolean flag to direct representatives parameter |
src/main/resources/templates/fragments/general-one-s-report-page-content/general-one-s-report-page-content.css |
Added styling for representatives picker container with flex layout and grid column spanning |
src/main/resources/templates/fragments/general-one-s-report-page-content/fragments/representatives-picker/representatives-picker.html |
Changed fragment signature to accept representatives list, fixed CSS path, removed inline style, added server-side rendering of representative checkboxes, updated script loading |
src/main/resources/templates/fragments/desktop-topbar/desktop-topbar.css |
Enhanced logo styling with flexbox centering, background, border, padding, and cursor pointer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| class="general-one-s-report-page-content__render-representatives-picker" | ||
| > | ||
| <div | ||
| th:replace="~{fragments/general-one-s-report-page-content/fragments/representatives-picker/representatives-picker :: representatives-picker( | ||
| userRole=${userRole}, | ||
| representatives=${representatives} | ||
| )}" | ||
| ></div> | ||
| </div> |
There was a problem hiding this comment.
The representatives picker wrapper div is now unconditionally rendered regardless of whether representatives data exists. While the inner content has th:if="${representatives}", the wrapper div with class general-one-s-report-page-content__render-representatives-picker will always be in the DOM even when empty. Consider adding th:if="${representatives}" to this wrapper div to avoid rendering unnecessary empty elements.
| "Наталія Шевченко" | ||
| ).collect(Collectors.toList()); | ||
|
|
||
|
|
There was a problem hiding this comment.
There are two consecutive blank lines here. Consider removing one to maintain consistent spacing throughout the codebase.
| <!-- grid-column: 1 / -1; --> | ||
|
|
There was a problem hiding this comment.
This comment appears to be a note about the CSS styling of the representatives picker element above, but its placement after the element is confusing. If this is meant to document the CSS property grid-column: 1 / -1, consider either removing it (as the CSS is self-explanatory) or moving it to be an inline comment near the class attribute of the element it refers to.
| <!-- grid-column: 1 / -1; --> |
| <div | ||
| class="general-one-s-report-page-content__render-representatives-picker" | ||
| > | ||
| <div | ||
| th:replace="~{fragments/general-one-s-report-page-content/fragments/representatives-picker/representatives-picker :: representatives-picker( | ||
| userRole=${userRole}, | ||
| representatives=${representatives} | ||
| )}" | ||
| ></div> | ||
| </div> |
There was a problem hiding this comment.
The representatives picker is now inside the general-one-s-report-page-content__toolbar-grid div which is conditionally rendered with th:if="${renderDatePicker or renderClientPicker}" at line 23. This means the representatives picker will not be displayed if both renderDatePicker and renderClientPicker are false, even when representatives data is available. Consider whether the representatives picker should have independent visibility logic or if the th:if condition at line 23 should include a check for representatives.
| renderDatePicker=true, | ||
| renderClientPicker=true, | ||
| renderRepresentativesPicker=true, | ||
| activeNav='accounts-overdue-debts', |
There was a problem hiding this comment.
The activeNav variable is defined but no longer used. It was previously used to set the active navigation item but has been replaced by activePage='overdue-debts' in the responsive-layout call at line 34. This unused variable should be removed to avoid confusion.
| activeNav='accounts-overdue-debts', |
| > | ||
| <div | ||
| id="representativesFilter" | ||
| class="representatives-picker__filter" |
There was a problem hiding this comment.
The removal of style="display: none" from the representativesFilter element may cause it to be visible before the JavaScript loads and determines whether to show it. The JavaScript code at representatives-picker.js:539 and representatives-picker.js:587,604 manages the visibility of this element. Consider whether this initial visibility is intentional or if CSS should be used to hide it by default.
| class="representatives-picker__filter" | |
| class="representatives-picker__filter" | |
| style="display: none" |
| List<String> representatives = Stream.of( | ||
| "Іван Петренко", | ||
| "Марія Коваленко", | ||
| "Олег Сидоренко", | ||
| "Наталія Шевченко" | ||
| ).collect(Collectors.toList()); |
There was a problem hiding this comment.
The representatives list is hardcoded with test data. For a production-ready implementation, this data should be retrieved from a service or database rather than being hardcoded in the controller. Note that there's already a separate API endpoint at /api/sales/plan/representatives (in SalesPlanController.java) that returns the same representatives list - consider whether this endpoint should be used instead.
| ).collect(Collectors.toList()); | ||
|
|
||
|
|
||
| model.addAttribute("userRole", "OWNER"); |
There was a problem hiding this comment.
The userRole is hardcoded as "OWNER". For a production-ready implementation, this should be retrieved from the authenticated user's session or security context rather than being hardcoded.
1 participant
No description provided.