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

forgetting browser warning #928

Merged
merged 7 commits into from
Apr 16, 2021
Merged

forgetting browser warning #928

merged 7 commits into from
Apr 16, 2021

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented Apr 14, 2021

closes #904

We might want to remove the text above forget the browser button in expansion panel since we already have the warning.

@render
Copy link

render bot commented Apr 14, 2021

@render
Copy link

render bot commented Apr 14, 2021

@render
Copy link

render bot commented Apr 14, 2021

@tanmoyAtb tanmoyAtb self-assigned this Apr 14, 2021
@tanmoyAtb tanmoyAtb marked this pull request as ready for review April 14, 2021 17:06
@tanmoyAtb tanmoyAtb requested review from FSM1, RyRy79261 and Tbaut April 14, 2021 17:06
@FSM1
Copy link
Contributor

FSM1 commented Apr 15, 2021

closes #904

We might want to remove the text above forget the browser button in expansion panel since we already have the warning.

Code works great. I think the text update in the browser panel should be included in this.

@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Apr 15, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Couple suggestions for naming and code removing the text above the delete button.
Also more importantly:
The modal doesn't close upon deleting a browser. You need to set back the flag to false in onDeleteShare.

@tanmoyAtb tanmoyAtb requested review from RyRy79261 and Tbaut April 15, 2021 15:31
@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels Apr 15, 2021
@Tbaut
Copy link
Collaborator

Tbaut commented Apr 15, 2021

@tanmoyAtb did you forget to push maybe :) ?

@tanmoyAtb
Copy link
Contributor Author

@tanmoyAtb did you forget to push maybe :) ?

all updated now :)

@tanmoyAtb tanmoyAtb added Status: Review Needed 👀 Added to PRs when they need more review and removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels Apr 16, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Awesome, just a little comment, but looks great otherwise.

…s/BrowserPanel.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Copy link
Contributor

@FSM1 FSM1 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 💯

@Tbaut Tbaut removed the Status: Review Needed 👀 Added to PRs when they need more review label Apr 16, 2021
@tanmoyAtb tanmoyAtb merged commit 68e799e into dev Apr 16, 2021
@tanmoyAtb tanmoyAtb deleted the fix/saved-browsers-warning-904 branch April 16, 2021 12:46
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.

Inform users that "forgetting a browser" means that the linked mnemonic will not work
4 participants