UI fixes #54
UI fixes #54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the ResourceEdits Show page to improve maintainability by extracting large template sections into reusable components and migrating from PrimeVue's legacy TabView component to the newer Tabs API. The changes also include minor UI enhancements to the BackButton component and fixes to the DropdownLink component to support different element types.
- Extracted monolithic template into 5 focused, reusable components (HeaderSection, SplitViewTab, DiffViewTab, FieldDisplay, ApprovalActions)
- Migrated from deprecated PrimeVue TabView/TabPanel to modern Tabs/TabList/Tab/TabPanels API
- Enhanced BackButton component to support slot content for additional text
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| resources/js/Pages/ResourceEdits/Show.vue | Refactored to use extracted components and new PrimeVue Tabs API, reducing template complexity |
| resources/js/Components/ResourceEdits/HeaderSection.vue | New component extracting header with back button, title, description, user profile, and merge button |
| resources/js/Components/ResourceEdits/SplitViewTab.vue | New component for side-by-side comparison of proposed vs current resource versions |
| resources/js/Components/ResourceEdits/FieldDisplay.vue | New reusable component for displaying field values (images, tags, or text) |
| resources/js/Components/ResourceEdits/DiffViewTab.vue | New component for displaying field-by-field differences |
| resources/js/Components/ResourceEdits/ApprovalActions.vue | New component extracting approval/voting UI |
| resources/js/Components/Navigation/BackButton.vue | Enhanced to support slot content and added visual separator |
| resources/js/Components/Navigation/Navbar.vue | Updated active state logic for Resources dropdown to include resource_edits.index route |
| resources/js/Components/Navbar/UserDropdown.vue | Added explicit method="POST" attribute to logout form |
| resources/js/Components/DropdownLink.vue | Refactored to support button, anchor, and Link elements via conditional rendering |
| resources/js/Pages/ResourceEdits/Create.vue | Minor TODO comment clarification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button v-if="as == 'button'" :class="classVal" class="w-full text-start"> | ||
| <slot /> | ||
| </button> | ||
|
|
||
| <a v-else-if="as == 'a'" :class="classVal" class="w-full text-start" :href="href"> |
There was a problem hiding this comment.
The equality comparison should use strict equality (===) instead of loose equality (==). This is a best practice in JavaScript to avoid type coercion issues.
| <button v-if="as == 'button'" :class="classVal" class="w-full text-start"> | |
| <slot /> | |
| </button> | |
| <a v-else-if="as == 'a'" :class="classVal" class="w-full text-start" :href="href"> | |
| <button v-if="as === 'button'" :class="classVal" class="w-full text-start"> | |
| <slot /> | |
| </button> | |
| <a v-else-if="as === 'a'" :class="classVal" class="w-full text-start" :href="href"> |
| as: String, | ||
| }); | ||
| const classVal = "block px-4 py-2 text-sm leading-5 text-gray-700 dark:text-gray-300 hover:bg-gray-100 dark:hover:bg-gray-800 focus:outline-none focus:bg-gray-100 dark:focus:bg-gray-800 transition duration-150 ease-in-out"; |
There was a problem hiding this comment.
The variable name "classVal" is unclear and doesn't follow common naming conventions. Consider renaming it to something more descriptive like "sharedClasses" or "dropdownLinkClasses" to better communicate its purpose.
| }); | ||
| // TODO: MOVE TO AXIOS BECAUSE REDIRECT FROM SUCCESSES WILL NOT CLEAR STORAGE, CONFIRM WITH TEST | ||
| // TODO: MOVE TO AXIOS BECAUSE REDIRECT FROM SUCCESSES WILL NOT CLEAR STORAGE, CONFIRM WITH TEST IN FUTURE |
There was a problem hiding this comment.
Spelling error: "SUCCESSES" should be "SUCCESS". The word "successes" (plural) doesn't make sense in this context where you're referring to successful redirects.
| // TODO: MOVE TO AXIOS BECAUSE REDIRECT FROM SUCCESSES WILL NOT CLEAR STORAGE, CONFIRM WITH TEST IN FUTURE | |
| // TODO: MOVE TO AXIOS BECAUSE REDIRECT FROM SUCCESS WILL NOT CLEAR STORAGE, CONFIRM WITH TEST IN FUTURE |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.