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

Security settings #877

Merged
merged 20 commits into from
Apr 8, 2021
Merged

Security settings #877

merged 20 commits into from
Apr 8, 2021

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Apr 6, 2021

closes #846

@render
Copy link

render bot commented Apr 6, 2021

@render
Copy link

render bot commented Apr 6, 2021

@render
Copy link

render bot commented Apr 6, 2021

@Tbaut Tbaut requested review from RyRy79261 and FSM1 April 7, 2021 10:43
@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Apr 7, 2021
@Tbaut Tbaut requested a review from tanmoyAtb April 7, 2021 10:43
@Tbaut Tbaut marked this pull request as ready for review April 7, 2021 10:43
@Tbaut Tbaut requested a review from FSM1 April 7, 2021 17:46
@RyRy79261
Copy link
Contributor

@Tbaut There seems to be a max limit set on the Account Security items instead of full width
image

@RyRy79261 RyRy79261 self-requested a review April 8, 2021 05:58
Copy link
Contributor

@RyRy79261 RyRy79261 left a comment

Choose a reason for hiding this comment

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

Desktop:
[1/4] just the previous comment about the widths of the account security items

Mobile:
[2/4] Wallet settings needs padding:
image

[3/4] There's also a lot of white space at the bottom of wallet settings
image

[4/4] And I'm not sure how to go back to the option selection screen on mobile

@RyRy79261 RyRy79261 added the Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. label Apr 8, 2021
@Tbaut
Copy link
Collaborator Author

Tbaut commented Apr 8, 2021

  1. I've set the grid to have width 8 on "md" because it looks pretty bad otherwise. Even with a 10 (see below), it doesn't look good IMHO, the item descriptions is just too far from the property.
    image

  2. and 3. fixed.

  3. I've stick to the design here, there's nothing to click to go back, but on mobile using the browser "back" should work and I thought it is intuitive enough. That's something we should think about with @sweetpea22 and see if real users have troubles with that.

@Tbaut Tbaut requested a review from RyRy79261 April 8, 2021 08:23
@Tbaut Tbaut removed the Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. label Apr 8, 2021
Thibaut Sardan and others added 2 commits April 8, 2021 10:24
* shares

* browserShares

* hasPassword and hasMnemonic
@FSM1
Copy link
Contributor

FSM1 commented Apr 8, 2021

  1. I've stick to the design here, there's nothing to click to go back, but on mobile using the browser "back" should work and I thought it is intuitive enough. That's something we should think about with @sweetpea22 and see if real users have troubles with that.

Yeah agreed that this can be dealt with later... One potential solution could be to use the breadcrumb to go back Home > Settings > Security

image

@tanmoyAtb
Copy link
Contributor

The setup password section just feels a little awkward with the spacings inside,
I get we are using the same component, maybe we could pass in a prop to make the design more like in figma.

Screenshot 2021-04-08 at 8 45 42 PM

@tanmoyAtb
Copy link
Contributor

Needs some spaces above.
Screenshot 2021-04-08 at 8 48 59 PM

Not so major,
In Tab size screens things get very cramped, maybe pull up the mobile view in tab size screens
Screenshot 2021-04-08 at 8 48 41 PM

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Wonderful work !
Everything seems to be in order.
Left some UI related comments which can be addressed later if we want.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Apr 8, 2021

Agreed the design is far from perfect. Let me try to get this sorted a bit before merging. The same components are used but this shouldn't prevent me from doing something nice.

@FSM1
Copy link
Contributor

FSM1 commented Apr 8, 2021

Wonderful work !
Everything seems to be in order.
Left some UI related comments which can be addressed later if we want.

I am also of the opinion that this should get merged as functionally it works really well, and we can sort out less critical styling and layout issues after release of all the latest to staging...

@Tbaut
Copy link
Collaborator Author

Tbaut commented Apr 8, 2021

I couldn't figure out the "cramped" around 1000px resolution, this is due to the grid :( so creating an issue out of it.
The spacing for the password form should be better now:
image

@Tbaut Tbaut requested review from RyRy79261 and removed request for RyRy79261 April 8, 2021 16:22
Co-authored-by: Tanmoy Basak Anjan <tanmoy3399@gmail.com>
@FSM1 FSM1 merged commit 336a1c3 into dev Apr 8, 2021
@FSM1 FSM1 deleted the feat/tbaut-settings-security-846 branch April 8, 2021 16:27
FSM1 added a commit that referenced this pull request Apr 12, 2021
Crash when going back after having selected Google auth #860 (PR #871)
Login optimizations (PR #879)
Minor fixes + Updates to color manipulator (#874)
Merge pull request #879 from ChainSafe/fix/login-optimizations
Merge pull request #887 from ChainSafe/fix/tbaut-hover-color
Merge pull request #886 from ChainSafe/feat/tkey-darkmode
Remove code duplication (#889)
Merge pull request #895 from ChainSafe/fix/select-all-892
Merge pull request #896 from ChainSafe/fix/mouse-highlight-issue-890
Merge pull request #894 from ChainSafe/fix/mobile-spinner-893
Merge pull request #877 from ChainSafe/feat/tbaut-settings-security-846
folder validation (#899)
saved browsers settings (#865)
Prevent the whole app from rerendering every 5s (#909)
Merge pull request #911 from ChainSafe/fix/share-transfer-modal-on-ta… …
Merge pull request #905 from ChainSafe/fix/bin-file-view-873 …
Merge pull request #906 from ChainSafe/feat/home-folder-nav-885 …
Merge pull request #907 from ChainSafe/fix/search-bucket-to-reset-on-…
Fix: Search results all show as files (#914) …
Fix double click opening the next file preview (#915) …
Rename tooltip and fixes (#912) …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a /settings/security route with a new security tab to setup new shares
4 participants