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

fixed the local ids selecting issue #13256

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

yashrajjain726
Copy link
Contributor

Fixed #11428

@yashrajjain726
Copy link
Contributor Author

@mertalev Please add someone to review

@yashrajjain726
Copy link
Contributor Author

yashrajjain726 commented Oct 9, 2024

@shenlong-tanwen updated the implementation inside the deleteLocalOnlyAssets func:

@yashrajjain726
Copy link
Contributor Author

@shenlong-tanwen Any update on review ?

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

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

You could do one of the following:

i) You could use your previous implementation of having the filtering logic in the widget and remove all the filtering logic in the provider as it is redundant and has errors
ii) Fix the logical issue in the provider and remove your filtering logic and toast handling in the widget as the assets are already filtered in the provider

Also fix any issues from the dart analyzer as well

Apologies for not being more clearer with the previous comment. Thanks for working on this.

@yashrajjain726
Copy link
Contributor Author

Thanks @shenlong-tanwen , I have updated the logic as per requirement. And, also removed UI logic from the provider code, as you mentioned and have implemented it from code perpective

Comment on lines +230 to +232

// Trigger a refresh to update the UI after deletion
await ref.read(assetProvider.notifier).getAllAsset();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

@yashrajjain726 Gentle reminder

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

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

Kindly fix the dart analysis issues. i.e, there are unused imports in the provider file

@shenlong-tanwen
Copy link
Member

shenlong-tanwen commented Oct 20, 2024

@yashrajjain726 The changes are good. Can you respond to the only remaining comment on why the refresh is needed?

@yashrajjain726
Copy link
Contributor Author

yashrajjain726 commented Oct 20, 2024

@shenlong-tanwen I have replied to it long time ago, please check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(mobile): delete from device doesn't work properly
3 participants