Bug 38854 - Unused 'class' prop in ToolbarButton
Summary: Unused 'class' prop in ToolbarButton
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Matt Blenkinsop
URL:
Keywords:
Depends on: 34215
Blocks: 38664
  Show dependency treegraph
 
Reported: 2025-01-09 13:29 UTC by Jonathan Druart
Modified: 2025-01-27 10:50 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38854: Remove 'class' property from ToolbarButton (7.86 KB, patch)
2025-01-09 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38854: Remove 'class' property from ToolbarButton (7.08 KB, patch)
2025-01-13 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38854: Remove 'class' property from ToolbarButton (7.12 KB, patch)
2025-01-13 16:25 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 38854: Revert incorrect changes (1.84 KB, patch)
2025-01-22 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38854: Remove 'class' property from ToolbarButton (6.80 KB, patch)
2025-01-23 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-01-09 13:29:32 UTC
New version of prettier fails on ToolbarButton because of (I guess) what it thinks is a reserver keyword.

[error] koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue: SyntaxError: Unexpected token, expected "{" (1:6)
[error] > 1 | class
[error]     |      ^

The error does not really make sense, the code is:
        class: {
            type: String,
            default: "btn btn-default",
        },

A solution could be to rename the property, but actually we never use another value than the default one. The easiest seems to simply remove this property.
Comment 1 Jonathan Druart 2025-01-09 13:29:51 UTC
% prettier --version 
3.4.2
Comment 2 Jonathan Druart 2025-01-09 13:32:33 UTC
Created attachment 176292 [details] [review]
Bug 38854: Remove 'class' property from ToolbarButton

New version of prettier fails on ToolbarButton because of (I guess) what it thinks is a reserver keyword.

[error] koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue: SyntaxError: Unexpected token, expected "{" (1:6)
[error] > 1 | class
[error]     |      ^

The error does not really make sense, the code is:
        class: {
            type: String,
            default: "btn btn-default",
        },

A solution could be to rename the property, but actually we never use another value than the default one. The easiest seems to simply remove this property.

% prettier --version
3.4.2

Test plan:
Upgrade prettier (yarn add prettier), confirm you have a more recent
version
Try to tidy the file using
  `prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue`

Without this patch you get the "Unexpected token" error.
With this patch apply you don't

Also rebuild the dist files using `yarn js:build` and confirm that the
buttons in the toolbar of the ERM and Preservation modules still appears
and are styled like before.
Comment 3 Leo Stoyanov 2025-01-09 16:37:21 UTC
Hi Jonathan,

After following the test plan, I wasn't able to recreate the bug. Specifically, even with the latest version of prettier (3.4.2) downloaded and even without the patch applied, there was no "SyntaxError: Unexpected token".

Note that I am on a Windows 11 machine running WSL with an Ubuntu distribution. My development environment includes Docker Desktop for containerization, and the Koha testing environment is running inside WSL.

Leo
Comment 4 David Nind 2025-01-09 18:08:54 UTC
I attempted to test, but don't get the error before the patch.

1. I ran: 

yarn add prettier
yarn add v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning "@redocly/cli > redoc > @cfaester/enzyme-adapter-react-18@0.8.0" has unmet peer dependency "enzyme@^3.11.0".
warning " > eslint-config-prettier@9.1.0" has unmet peer dependency "eslint@>=7.0.0".
warning " > eslint-plugin-prettier@5.1.3" has unmet peer dependency "eslint@>=8.0.0".
warning "swagger-cli > @apidevtools/swagger-cli > @apidevtools/swagger-parser@10.1.0" has unmet peer dependency "openapi-types@>=7".
[4/4] Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
info Direct dependencies
└─ prettier@3.4.2
info All dependencies
└─ prettier@3.4.2
Done in 58.41s.

2. Prettier version after update:

prettier --version
3.4.2

3. Output:

prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue
<template>
    <router-link :to="to" :class="class"
        ><font-awesome-icon v-if="icon" :icon="icon" /> {{ title }}</router-link
    >
</template>

<script>
export default {
    props: {
        to: {
            type: [String, Object],
        },
        class: {
            type: String,
            default: "btn btn-default",
        },
        icon: {
            type: String,
            required: false,
        },
        title: {
            type: String,
        },
    },
    name: "Toolbar",
}
</script>
Comment 5 Matt Blenkinsop 2025-01-10 11:36:09 UTC
Same as the above - no errors. Are you using a prettier.config.js file in your branch with any additional config extending the defaults?
Comment 6 Matt Blenkinsop 2025-01-10 11:40:37 UTC
Or made any further changes in .prettierrc.js?
Comment 7 Jonathan Druart 2025-01-13 09:12:20 UTC
(In reply to Matt Blenkinsop from comment #6)
> Or made any further changes in .prettierrc.js?

Good catch!
I indeed had an incorrectly-positioned config entry in .prettierrc.js (context is bug 38664).

This is invalid, sorry for the noise!
Comment 8 Jonathan Druart 2025-01-13 13:11:22 UTC
The patch can still be useful as it removes an unused property. What do you think Matt? Keeping for flexibility or removing to keep the code clean?

It's also confusing to pass a value when it's actually the default value.
Comment 9 Matt Blenkinsop 2025-01-13 13:57:52 UTC
I think we can remove it - if it needs adding back in at any point then we can make sure to use terminology that doesn't conflict with reserved keywords
Comment 10 Jonathan Druart 2025-01-13 14:27:38 UTC
(In reply to Matt Blenkinsop from comment #9)
> I think we can remove it - if it needs adding back in at any point then we
> can make sure to use terminology that doesn't conflict with reserved keywords

ok thanks for confirming. I have adjusted the bug title.
Comment 11 Jonathan Druart 2025-01-13 14:28:58 UTC
Created attachment 176450 [details] [review]
Bug 38854: Remove 'class' property from ToolbarButton

We never use another value than the default one.
We can simply remove this property.

Test plan:
Rebuild the dist files using `yarn js:build` and confirm that the
buttons in the toolbar of the ERM and Preservation modules still appears
and are styled like before.
Comment 12 William Lavoie 2025-01-13 16:25:51 UTC
Created attachment 176457 [details] [review]
Bug 38854: Remove 'class' property from ToolbarButton

We never use another value than the default one.
We can simply remove this property.

Test plan:
Rebuild the dist files using `yarn js:build` and confirm that the
buttons in the toolbar of the ERM and Preservation modules still appears
and are styled like before.

Signed-off-by: William Lavoie <william.lavoie@inLibro.com>
Comment 13 Matt Blenkinsop 2025-01-20 11:31:08 UTC
Looks like there are two errors:
1) AgreementsShow the class has been removed from an <a> tag that isn't part of the ToolbarButton component. The Delete button is styled incorrectly as a result
2) TrainsShow - the ToolbarButton component for editing trains has now got a '_class' property instead of having the 'class' removed (lines 93-101)
Comment 14 Jonathan Druart 2025-01-22 11:16:07 UTC
Created attachment 176903 [details] [review]
Bug 38854: Revert incorrect changes
Comment 15 Jonathan Druart 2025-01-22 11:16:20 UTC
(In reply to Matt Blenkinsop from comment #13)
> Looks like there are two errors:
> 1) AgreementsShow the class has been removed from an <a> tag that isn't part
> of the ToolbarButton component. The Delete button is styled incorrectly as a
> result

Oops, good catch!

> 2) TrainsShow - the ToolbarButton component for editing trains has now got a
> '_class' property instead of having the 'class' removed (lines 93-101)

Oops again, that was part of the first attempt.

Thanks!
I've added a follow-up patch to help the review, but please squash them when QAing.
Comment 16 Matt Blenkinsop 2025-01-23 14:39:43 UTC
Created attachment 176971 [details] [review]
Bug 38854: Remove 'class' property from ToolbarButton

We never use another value than the default one.
We can simply remove this property.

Test plan:
Rebuild the dist files using `yarn js:build` and confirm that the
buttons in the toolbar of the ERM and Preservation modules still appears
and are styled like before.

Signed-off-by: William Lavoie <william.lavoie@inLibro.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 17 Matt Blenkinsop 2025-01-23 14:40:08 UTC
Squashed and PQA
Comment 18 Katrin Fischer 2025-01-24 10:56:12 UTC
This no longer applies, can you please check?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 38854: Revert incorrect changes
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsShow.vue).
error: could not build fake ancestor
Patch failed at 0001 Bug 38854: Revert incorrect changes
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-38854-Revert-incorrect-changes-cicgpxvi.patch
Comment 19 Matt Blenkinsop 2025-01-24 11:08:56 UTC
Sorry, forgot to obsolete the squashed patch
Comment 20 Matt Blenkinsop 2025-01-24 11:09:17 UTC
Try again, should be fine now
Comment 21 Katrin Fischer 2025-01-24 14:04:57 UTC
The dependecy is not even signed off yet :(
Comment 22 Katrin Fischer 2025-01-24 14:05:16 UTC
(In reply to Katrin Fischer from comment #21)
> The dependecy is not even signed off yet :(

Obv wrong bug, please ignore.
Comment 23 Katrin Fischer 2025-01-24 15:15:21 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 24 Pedro Amorim 2025-01-27 10:50:20 UTC
Part of the work done here, plus abstracting the component(s) to prevent things like these from being needed (change the same thing everywhere) has already been done in bug 37301 for many months.
Any chance we can get some eyes on that bug?