https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38201
--- Comment #4 from Jonathan Druart <[email protected]> --- 1. I think this is problematic for translatability: + {{ $__("Remove this %s").format(resourceName) }}</a + {{ $__("Add new %s").format(resourceName) }}</a + $__("There are no %s created yet").format(resourceName) 2. http://localhost:8081/cgi-bin/koha/preservation/trains/1 a. Toolbar button "Add items" is not correctly styled: https://snipboard.io/W3Be1Y.jpg b. warning in the console: [Vue warn]: Property "class" was accessed during render but is not defined on instance. at <Toolbar key=0 to= Object { name: "TrainsFormAddItem", params: {…} } icon="plus" ... > 3. Create a train, then delete it from the list view a. The table is not refreshed and the train is still displayed b. warning in the console: [Vue Router warn]: Discarded invalid param(s) "train_item_id" when navigating. See https://github.com/vuejs/router/blob/main/packages/router/CHANGELOG.md#414-2022-08-22 for more details. 4. AgreementResource.vue + nameLC: __("user"), + nameUC: __("Agreement user"), + namePL: __("users"), LC, UC, PL? Can it be verbose so we don't need to guess what it means? 5. Update users for an agreement: https://snipboard.io/FywJhr.jpg Change one field and all are updated, wrong binding here. Oops, it actually happens for other linked resources. See also the duplication of the error there: https://snipboard.io/jmvBbL.jpg 6. (In reply to Pedro Amorim from comment #3) > Chatting with Matt, this should likely be based on bug 37930. It is currently not. And bugs encountered on bug 37301 are fixed here. Can we have a clear tree? 7. Is there a TODO list? It seems that other components need adjustment to use this change (the remaining FormAdd, List, Show components). What's missing to make it ready for testing/QA and push? 8. With the additional abstractions everything is harder to read IMO, we should add documentation, at least for the "Base" components. 9. There is way too many changes for a single bug report, please split into smaller chunks next time. It's really hard to follow. 10. Cypress tests are failing: Spec Tests Passing Failing │ ✖ AdditionalFields_spec.ts 02:22 12 2 10 │ ✖ InfiniteScrollSelect_spec.ts 00:56 4 - 4 │ ✖ Admin/RecordSources_spec.ts 00:51 5 1 4 │ ✖ ERM/Agreements_spec.ts 01:06 5 - 5 │ ✖ ERM/Licenses_spec.ts 01:04 5 - 5 │ ✖ ERM/UserRoles_spec.ts 00:15 1 - 1 11. I cannot save an agreement with a license 400 - Object { message: "Expected string - got null.", path: "/body/agreement_licenses/0/status" } ==== Overall I think the idea is good but it adds some "magic" that won't help new developer to grasp our VueJS code. I feel like you have been too far, and things won't be as easy to maintain as you think. This is something weird: 310 { 311 name: "ended_on", 312 type: "component", 313 label: __("End date"), 314 componentPath: "./FlatPickrWrapper.vue", 315 required: false, 316 props: { 317 id: { 318 type: "string", 319 value: "ended_on_", 320 indexRequired: true, 321 }, 322 }, 323 }, I expect more to see something like "type: date", and not a component path to the flatpickr component. Same for this: 122 { 123 name: "vendor_id", 124 type: "component", 125 label: __("Vendor"), 126 showElement: { 127 type: "text", 128 value: "vendor.name", 129 link: { 130 href: "/cgi-bin/koha/acqui/supplier.pl", 131 params: { 132 bookseller_id: "vendor_id", 133 }, 134 }, 135 }, 136 componentPath: "./FormSelectVendors.vue", 137 props: { 138 id: { 139 type: "string", 140 value: "agreement_id_", 141 indexRequired: true, 142 }, 143 }, 144 }, This should be shorter, you don't want to duplicate this whenever a component will be linked to a vendor. It could be "vendor_id is a vendor, and the value for this component is vendor.name, and is required". The other info will be copy/pasted, and increase the risk of regressions, inconsistencies, etc. The 500 lines of definition of the agreement (resourceAttrs in AgreementResource.vue) are not trivial to understand, neither to know what does what. I think we could make it a bit more readable by moving some redundant code that is in there: the relationshipWidget are defined in LicenseResource and AgreementResource for instance. We should make sure the order of the attributes are always the same (how??), otherwise it get messy, for instance: 339 name: "notes", 340 required: false, 341 type: "text", 342 label: __("Notes"), vs 421 name: "role", 422 type: "select", 423 label: __("Role"), 424 avCat: "av_user_roles", 425 required: true, This pattern looks wrong: 482 resourceRelationships: { 483 type: "resourceProperty", 484 resourceProperty: "agreement_licenses", 485 }, But I don't have anything else to suggest now. Sorry for the messy notes... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
