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

fix: added error message for change password #2025

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

ShivamJhaa
Copy link
Contributor

@ShivamJhaa ShivamJhaa commented Jan 30, 2023

Describe the changes you have made in this PR

Added error message when we try to update the password and alby account is locked.

Link this PR to an issue [optional]

Fixes _#1632

Type of change

(Remove other not matching type)

  • fix: Bug fix (non-breaking change which fixes an issue)

Screenshots of the changes [optional]

alby.mp4

Add screenshots to make your changes easier to understand. You can also add a video here.

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@ShivamJhaa
Copy link
Contributor Author

@im-adithya Can you please review this PR and start the CI test for the PR?
Thanks!

@rolznz
Copy link
Contributor

rolznz commented Jan 30, 2023

Is it possible when logging out to make the entire extension tab inaccessible? it seems strange to log out and still have access to the settings page.

@github-actions
Copy link

github-actions bot commented Jan 30, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@im-adithya
Copy link
Member

Thanks for the PR @ShivamJhaa!

However, as @rolznz suggested, this page shouldn't appear whenever the user locks the extension. So instead of showing an error toast, can you modify the code to close all the extension tabs on lock?

Here's the lock function: https://github.com/getAlby/lightning-browser-extension/blob/master/src/extension/background-script/state.ts#L119

You can fetch all the currently open extension tabs and close them there. See https://developer.chrome.com/docs/extensions/reference/extension/#method-getViews

Again, thanks for letting us know about the issue; let me know if you need help!

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

thanks for the PR!

src/app/screens/Settings.tsx Outdated Show resolved Hide resolved
@bumi
Copy link
Collaborator

bumi commented Jan 30, 2023

@im-adithya @ShivamJhaa I'd do both. catching a potential error and showing an error message is still a good idea I think. maybe something else goes wrong, so at least the user knows.

@ShivamJhaa
Copy link
Contributor Author

Thanks for the PR @ShivamJhaa!

However, as @rolznz suggested, this page shouldn't appear whenever the user locks the extension. So instead of showing an error toast, can you modify the code to close all the extension tabs on lock?

Here's the lock function: https://github.com/getAlby/lightning-browser-extension/blob/master/src/extension/background-script/state.ts#L119

You can fetch all the currently open extension tabs and close them there. See https://developer.chrome.com/docs/extensions/reference/extension/#method-getViews

Again, thanks for letting us know about the issue; let me know if you need help!

Thanks for the help, I have made the changes as suggested. Please take a look.

alby2.mp4

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

ack

escapedcat

This comment was marked as duplicate.

@ShivamJhaa
Copy link
Contributor Author

@im-adithya Please take a look, I have resolved the merge conflicts and need some help over this comments #2025 (comment)
Thanks!

@ShivamJhaa
Copy link
Contributor Author

@im-adithya Done the changes as suggested.

@escapedcat escapedcat merged commit 0e8f2a4 into getAlby:master Feb 7, 2023
@ShivamJhaa ShivamJhaa deleted the fix/change_pass_issue branch February 7, 2023 13:24
This pull request was closed.
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