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: delete proof display #378

Merged
merged 3 commits into from
Feb 27, 2024
Merged

fix: delete proof display #378

merged 3 commits into from
Feb 27, 2024

Conversation

dq18
Copy link
Collaborator

@dq18 dq18 commented Feb 24, 2024

What

  • Created a store dedicated to proofs storing for now the list of user proofs in frontend with add and remove methods
  • Changed ProofDeleteChip to update this store with remove
  • Changed UserDashboardProofList to use the proofs list from the store

Further improvements

  • Move the api calls in the store ? We probably will use the same functions in several pages

Fixes bug(s)

  • Deleted proof takes up space Deleted proof takes up space #371
    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 to UserDashboardProofList which makes things harder to follow

@dq18 dq18 requested a review from raphodn February 24, 2024 21:17
@@ -0,0 +1,18 @@
import { defineStore } from 'pinia';
Copy link
Member

@raphodn raphodn Feb 24, 2024

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 ^^

Copy link
Member

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 ?)

Copy link
Collaborator Author

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...

Copy link
Member

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

Copy link
Collaborator Author

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

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
Copy link
Member

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)

Copy link
Member

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 🤔

Copy link
Member

@raphodn raphodn left a comment

Choose a reason for hiding this comment

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

Awesome !

@dq18 dq18 merged commit bc16444 into master Feb 27, 2024
2 checks passed
@dq18 dq18 deleted the fix-delete-proof-display branch February 27, 2024 18:00
@raphodn raphodn linked an issue Feb 27, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Deleted proof takes up space
2 participants