-
Notifications
You must be signed in to change notification settings - Fork 124
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: Allow cancelling updates in manage page #1536
Conversation
Hello @BLKKKBVSIK, can you please review this as well, when you get chance. Thanks. |
@spydon, @BLKKKBVSIK please review this when you get time.. |
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.
Not sure why the mocks are failing, can you try running melos generate
?
onPressed: updatesModel.refreshableSnapNames.isNotEmpty && | ||
!updatesModel.state.isLoading && | ||
updatesModel.activeChangeId != null |
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 would move this out to a variable with a good name so that the code gets more readability
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.
Thanks for suggestion, made the change to extract a variable.
Ran melos generate
, and it fixed the missing mock definition. Thanks.
Few tests aren't passing, once this will be resolved, LGTM |
@BLKKKBVSIK, thanks for review, fixed the tests now. Please have a look when you get chance. |
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.
Lgtm!
thanks for review @spydon. @BLKKKBVSIK please let me know if there is any thing I can do for this PR to be merged.. |
Hey @codinesh thanks for working on this 🙌 One thing to amend is that the cancel button should be hidden until the 'Update all' is trigged, as it can be misleading for the users to have a cancel button without context.
The width of the button is dependent on the text length, with a padding of 16px on both sides. Not 100% if this is what we have on Yaru, so please double-check. Cheers! |
Hello @anasereijo, Thanks for suggestions, I will take up the change to hide Cancel button while not processing updates. |
fix: Allow cancelling updates in manage page
Fixes #1533 by adding a Cancel button next to 'Update All' in manage page.
Cancel button is disabled by default, when no updates are not initialized.
Once update all is triggered and in progress, Cancel button will be enabled, and will stop the updates on click.
Hello @anasereijo, I couldn't find the link to Figma, to make the button width match with designs. Will be happy to take that change, please help me with Figma link and access.