-
Notifications
You must be signed in to change notification settings - Fork 0
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
star/un-star a conversation #28
Conversation
I'd like to use a star character that is more colorful, like this one: ⭐ |
I'm sorry, it's my fault that the Apollo client cache was not updating after mock responses. I explained that when Apollo reads a response from a mutation it can look up a value in the cache with the same type and id and update that value automatically, and I explained that responses must include an I pushed a change to add the necessary |
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.
This is great work! You have implemented a feature that touches nearly every part of the tech stack for this app, and you have shown good initiative.
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 lots of good progress here. Please remember to either remove or update the isStarred
calculation in models/conversation.ts
.
packages/client/src/Dashboard.tsx
Outdated
}) | ||
//if not all selected star, we want to star instead of unstar | ||
if (isStarred && selected.length > 1) { | ||
isStarred = |
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.
Assigning isStarred
twice is a code smell. The logic for this second assignment is correct - using every
instead of reduce
covers the cases that you need. The first assignment is redundant. You can delete the first assignment, and delare isStarred
with const
.
{isStarred ? <StarBorder /> : <StarIcon />} | ||
</IconButton> | ||
<Tooltip | ||
title="Archive Selected Conversation" |
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.
These tooltips are great! But please pluralize "Conversations" for the list view actions.
flag: handler({ | ||
enqueue(params: { accountId: ID; box: { name: string }; uids: number[] }) { | ||
cache.addFlag({ ...params, flag: "\\Flagged" }) | ||
setFlagged: handler({ |
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.
Good stuff. Would you remove the unFlag
handler now that it is not used anymore?
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.
More good progress! But there are some outstanding comments from my previous review.
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.