-
Notifications
You must be signed in to change notification settings - Fork 580
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
[wip] fix: Update tasks list using the relay store #11247
base: main
Are you sure you want to change the base?
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.
Looking good, one non-blocking comment that could make relay store operations unnecessary if possible
if (!!taskAcknowledged) { | ||
ConnectionHandler.deleteNode(tasksConnection, taskID) | ||
} |
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.
non-blocking: If it is possible to include the fragment that gets the list of tasks in your mutation gql fields, then the relevant store data will be updated without needing to get surgical (you could still use an optimistic update to temporarily remove that task). I think this would be something like here - ...HomeViewSectionTasks_section
so maybe not easy to access from this mutation.
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.
I was just gonna post the same thing @erikdstock! Can this be rolled up in the mutation query itself, in one quick fragment spread in the response?
(these days its rare to have to mutate the store manually)
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 code looks quite complex and hard to understand, and it seems like it could break very easily, which would break the entire dismiss logic in the component.
I would vote for either keeping it simple (like the initial not-100%-ideal solution with useState
) or trying to make it work with a fragment spread.
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.
I agree with this being not easy to ready, I'll try spreading the fragment in the query and see what happens :)
I'll pause the work on this since it needs some work on MP, and the current implementation has the |
This PR resolves []
Description
This PR is a follow-up on #10961
This PR updates the way we update the tasks list to utilize the Relay store
This change uses the Relay mutation
updater
function to remove a node from the tasks list if the dismiss/acknowledge mutation was successfulPR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.