-
-
Notifications
You must be signed in to change notification settings - Fork 13
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: delete proof display #378
Conversation
src/store/proofs.js
Outdated
@@ -0,0 +1,18 @@ | |||
import { defineStore } from 'pinia'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool to start storing data in the store ! Inspiration for #141 ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why in a seperate store ? Also, these are user proofs so could be under the user object, we could want to persist, and it avoid importing multiple stores, no ?
(sure in big apps they probably split stuff, but we don't need to introduce this complexity this soon, do we ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of this PR was also to start a discussion on separating stores, what should really be separated and how much data should be stored (if we do prices for example, there might be too much data).
I don't know the best practice, one thought was to have some kind of matching with backend models, so separate stores for User, Proof, Prices, Location, Stats...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can start the discussion once our store feels too big or complex ^^
I think we're far from that situation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up to use the unique store. Should be ready to merge
src/views/UserDashboardProofList.vue
Outdated
this.userProofTotal = data.total | ||
data.items.forEach(proof => this.appStore.addProof(proof)) | ||
this.appStore.user.proofs.sort((a, b) => new Date(b.created) - new Date(a.created)) | ||
this.userProofTotal = this.appStore.user.proofs.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is an issue with this line :
- let's suppose the user has 15 proofs
- but we only fetched his last 10 proofs
- the total will display 10 (the list length, though it should display 15)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a solution would be to keep the api response format in the store. or have the total in another key 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome !
What
ProofDeleteChip
to update this store with removeUserDashboardProofList
to use the proofs list from the storeFurther improvements
Fixes bug(s)
The original way of removing a proofCard by manipulating the DOM was giving issues and not recommended in Vue.
An alternative is to "bubble" an emit event from
ProofDeleteChip
toUserDashboardProofList
which makes things harder to follow