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

[wip] fix: Update tasks list using the relay store #11247

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MrSltun
Copy link
Member

@MrSltun MrSltun commented Dec 4, 2024

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 successful

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • Update tasks' list using the relay mutation updater

Need help with something? Have a look at our docs, or get in touch with us.

@MrSltun MrSltun self-assigned this Dec 4, 2024
@ArtsyOpenSource
Copy link
Contributor

This PR contains the following changes:

  • Dev changes (Update tasks' list using the relay mutation updater - MrSltun)

Generated by 🚫 dangerJS against 12a2179

Copy link
Contributor

@erikdstock erikdstock left a 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

Comment on lines +36 to +38
if (!!taskAcknowledged) {
ConnectionHandler.deleteNode(tasksConnection, taskID)
}
Copy link
Contributor

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.

Copy link
Member

@damassi damassi Dec 4, 2024

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)

Copy link
Contributor

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.

Copy link
Member Author

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

@MrSltun
Copy link
Member Author

MrSltun commented Dec 5, 2024

I'll pause the work on this since it needs some work on MP, and the current implementation has the useState implementation, I'll convert this PR to a draft one

@MrSltun MrSltun marked this pull request as draft December 5, 2024 14:19
@MrSltun MrSltun changed the title fix: Update tasks list using the relay store [wip] fix: Update tasks list using the relay store Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants