Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: lint Vue files too #772

Merged
merged 16 commits into from
Nov 28, 2022
Merged

fix: lint Vue files too #772

merged 16 commits into from
Nov 28, 2022

Conversation

guidojw
Copy link
Member

@guidojw guidojw commented Aug 27, 2022

Summary

We have the ESLint Vue plugin but weren't actually using it, this PR fixes that.

TODO:

  • Fix the vue/no-mutating-props error (I have no idea how (yet)).

@guidojw guidojw marked this pull request as draft August 27, 2022 22:56
@guidojw
Copy link
Member Author

guidojw commented Aug 30, 2022

@wilco375 Can you test this PR (in general) and specifically check if what I did in 17a4c11 is correct? The total amount of an order on the order screen should automatically update when you click + or - on a product.

@guidojw guidojw marked this pull request as ready for review August 30, 2022 19:53
@guidojw guidojw requested a review from wilco375 August 30, 2022 19:53
@guidojw guidojw changed the title fix(package): lint vue files too fix: lint vue files too Aug 30, 2022
Copy link
Contributor

@wilco375 wilco375 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the flash notification. Other than that everything seems to work as expected.

Copy link
Contributor

@wilco375 wilco375 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, updating a order doesn't seem to be working properly.

When updating an item, the total price of the order is not updated. It looks like you didn't actually add a listener for the emitted event (https://vuejs.org/guide/components/events.html#emitting-and-listening-to-events). Also, I think, in updateOrderTotal in ActivityOrders, you should edit the data that is passed to :items in b-table. So either refresh the data, or, more efficiently, store the orders as data, and edit that object.

@guidojw
Copy link
Member Author

guidojw commented Sep 1, 2022

Oops, updating a order doesn't seem to be working properly.

When updating an item, the total price of the order is not updated. It looks like you didn't actually add a listener for the emitted event (vuejs.org/guide/components/events.html#emitting-and-listening-to-events). Also, I think, in updateOrderTotal in ActivityOrders, you should edit the data that is passed to :items in b-table. So either refresh the data, or, more efficiently, store the orders as data, and edit that object.

I think I fixed the event handler in 6dd6698.

The way of updating you're talking about is already done I think, using the Vue slots magic (still cannot really wrap my head around that Vue feature).

@guidojw
Copy link
Member Author

guidojw commented Sep 1, 2022

(I still do not have a working sofia environment and at this point never will so please test)

@guidojw guidojw changed the title fix: lint vue files too fix: lint Vue files too Sep 1, 2022
@DrumsnChocolate
Copy link
Contributor

@guidojw how did you approach compliance with vue/no-mutating-props?

@guidojw
Copy link
Member Author

guidojw commented Oct 23, 2022

@guidojw how did you approach compliance with vue/no-mutating-props?

17a4c11 & 6dd6698
I did not test (any of) this

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Oct 23, 2022

@guidojw how did you approach compliance with vue/no-mutating-props?

17a4c11 & 6dd6698 I did not test this

that should work, but I'd prefer to use camel case in the event name. That is, emit string "updateOrderTotal" and then you should be able to use @updateOrderTotal or if that is not possible @update-order-total (vue sometimes does that hyphenation, idk why exactly)

@guidojw
Copy link
Member Author

guidojw commented Oct 23, 2022

@guidojw how did you approach compliance with vue/no-mutating-props?

17a4c11 & 6dd6698 I did not test this

that should work, but I'd prefer to use camel case in the event name. That is, emit string "updateOrderTotal" and then you should be able to use @updateOrderTotal or if that is not possible @update-order-total (vue does that hyphenation, idk why exactly)

I had it like that as you can see in the first commit, but then I found the other convention being used elsewhere in the codebase so changed it.

@guidojw
Copy link
Member Author

guidojw commented Oct 23, 2022

@guidojw how did you approach compliance with vue/no-mutating-props?

17a4c11 & 6dd6698 I did not test this

that should work, but I'd prefer to use camel case in the event name. That is, emit string "updateOrderTotal" and then you should be able to use @updateOrderTotal or if that is not possible @update-order-total (vue sometimes does that hyphenation, idk why exactly)

this.$emit('updateuser', user);

this.$emit('selectcash', 'pay_with_cash');

this.$emit('selectpin', 'pay_with_pin');

This is out of the scope of this PR but could altogether be changed in another one. I don't know what's a good way forward with that capitalization though as most DOM event names are fully lowercase too (mouseup, mousedown, mousemove, keyup, keydown, dblclick).

@DrumsnChocolate
Copy link
Contributor

@guidojw https://vuejs.org/guide/components/events.html#emitting-and-listening-to-events have a look at this.
I think that DOM event names should be kept the way they are, but custom events are definitely the place to use camelCase.
But of course, if all our other custom events are also lowercase, then we should keep using lowercase in this PR.
I wouldn't mind seeing this refactored in a future PR, but it's a very minor problem. I'll open a separate issue for this

Copy link
Contributor

@DrumsnChocolate DrumsnChocolate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird how github doesn't recognize renames very well.
Except for a couple of comments, lgtm. I'll approve when my comments are resolved

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Oct 23, 2022

@guidojw v-slots exist to allow you to swap out a piece of 'default' htmlvue template. This is very useful in case you want to render custom html for the row details of each row in a b-table, like we do several times throughout these files. The exact slots that are available to you are determined by the context, i.e. the component from which you would like to use a slot. If you are looking for some more clarification, let me know, I'm happy to tell you more about them

@guidojw
Copy link
Member Author

guidojw commented Oct 23, 2022

It's weird how github doesn't recognize renames very well.

Except for a couple of comments, lgtm. I'll approve when my comments are resolved

In what case? In the case of capitalisation changes, there's a git config you should set, I think it's 'ignoreCase'

@DrumsnChocolate
Copy link
Contributor

It's weird how github doesn't recognize renames very well.
Except for a couple of comments, lgtm. I'll approve when my comments are resolved

In what case? In the case of capitalisation changes, there's a git config you should set, I think it's 'ignoreCase'

I'm referring to github showing the renamed vue files as deleted and new, instead of just moved. at least that's what it shows for me, under Files Changed

@DrumsnChocolate
Copy link
Contributor

It's weird how github doesn't recognize renames very well.
Except for a couple of comments, lgtm. I'll approve when my comments are resolved

In what case? In the case of capitalisation changes, there's a git config you should set, I think it's 'ignoreCase'

I'm referring to github showing the renamed vue files as deleted and new, instead of just moved. at least that's what it shows for me, under Files Changed

image

@guidojw
Copy link
Member Author

guidojw commented Oct 24, 2022

I will not be updating this PR anymore, so @DrumsnChocolate or @wilco375 can one of you take it over?

Copy link
Contributor

@wilco375 wilco375 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DrumsnChocolate Is there anything that still needs to happen to this PR before it can be merged? Just checked it out again and everything seems to be working.

@wilco375 wilco375 enabled auto-merge (squash) November 28, 2022 20:48
@wilco375 wilco375 merged commit edad2dc into staging Nov 28, 2022
@wilco375 wilco375 deleted the fix/lint-vue-files branch November 28, 2022 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants