Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve device list in Security & Privacy settings #7004

Merged
merged 9 commits into from
Oct 29, 2021

Conversation

duxovni
Copy link
Contributor

@duxovni duxovni commented Oct 21, 2021

Overhaul the device list in the "Security and Privacy" settings tab to include device trust status, provide buttons for verifying unverified devices, and improve overall usability and style. Fixes element-hq/element-web#17767. Fixes element-hq/element-web#19360.

Screenshots

device-view-4-green-check
device-view-4-multiple
device-view-4-no-other
device-view-4-one-unverified
device-view-4-rename
device-view-4-selected
device-view-4-two-subsections


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://617bfcd8502bee42270f8ad9--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Overhaul the device list in the "Security and Privacy" settings tab to
include device trust status, provide buttons for verifying unverified
devices, and improve overall usability and style.
@duxovni duxovni requested a review from a team as a code owner October 21, 2021 16:39
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Overall this looks great - thanks for fixing it!

I've outlined a couple things in the comments, but for more overall things:

  • It looks like the tests are unhappy, largely due to the strings that went missing from the translation file
  • Screenshots in the PR description please :)
  • PR titles end up in user-facing changelogs, so we generally try to phrase them as though we're talking to the user. Suggestion would be "Improve devices section of settings" or similar.

src/components/views/settings/DevicesPanelEntry.tsx Outdated Show resolved Hide resolved
src/i18n/strings/en_EN.json Outdated Show resolved Hide resolved
@duxovni duxovni changed the title Implement device trust view in user settings Improve device list in Security & Privacy settings Oct 22, 2021
@turt2live turt2live self-requested a review October 22, 2021 14:58
@aaronraimist
Copy link
Contributor

This is mostly an improvement but this design prevents you from selecting multiple devices which is useful when you don't want to click sign out and enter your password multiple times. Please keep the option to select multiple devices. The "Sign out all other devices" button is nice but does not replace it for when you only want to sign out of some but not all other devices.

@czeidler
Copy link
Contributor

Looks cool! please feel free to close #5480

  • Regarding the comment from @aaronraimist. Maybe keep logged in for 5min or as long as the settings dialog is opened to avoid entering the password multiple times.
  • Think the currently selected/edited item should be highlighted...

Out of scope of this PR and more like a nice have. Maybe we should generate an icon from the device id/MAC and display it during registration and in this device list to make it easier to recognize devices. Just an idea...

@duxovni
Copy link
Contributor Author

duxovni commented Oct 25, 2021

The issue with signing out multiple devices separately is that it's entirely up to the homeserver implementation whether to prompt for reauthentication every time you delete another device, or offer a short grace period, or not ask for reauthentication at all. So we have no control over whether we'll need to enter a password. I see @aaronraimist's point, and I'll bring back the multi-select functionality.

@duxovni
Copy link
Contributor Author

duxovni commented Oct 25, 2021

Another thing that came up during an internal demo: signing out our own device should generate a warning prompt if there are no other devices and no secure backups, basically the same behavior as #5410 implements for the regular "Sign Out" button in the main dropdown menu. It also shouldn't require re-entering our password; rather than using the same session deletion API we use for signing out other devices, we can just sign out our own device directly.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Code looks fine to me :D

Someone from the design team will want to take a look as well.

@duxovni duxovni requested a review from amshakal October 28, 2021 05:35
@duxovni
Copy link
Contributor Author

duxovni commented Oct 28, 2021

@amshakal does this match what you had in mind during our design conversation about bringing back multi-select? (There's updated screenshots in the description.)

I don't think we talked about the "Select/Deselect all" buttons, but those felt like a useful thing to have; there's one for each subsection, and they only appear if that subsection has multiple devices.

@amshakal
Copy link

Thanks Faye, it's looking great! 👯 I have a few small comments though:

  1. I think it's getting hard to differentiate between the title and list of devices under it, so could we remove the device names from bold style to regular?
  2. For checkboxes, we tend to use green checkboxes when selected, I think we should stick to that design to match the design system.
  3. I think the placement of sign out button for all unverified devices if throwing me off, again it's because we are not creating enough of a heiarchy between titles and devices under each title. Should we move it to the bottom with the other sign out button?
  4. Consider removing 'no other devices' for empty state. I think we can do without mentioning that. What do you think?

I think the following changes might make it easier for a user to scan through the list. I have attached a screenshot to illustrate the changes I have mentioned above, let me know if these suggestions make sense. :)
Frame 3835
.

@duxovni
Copy link
Contributor Author

duxovni commented Oct 29, 2021

@amshakal this addresses most of the things we talked about. The only things left are

  • "No other devices": I forgot to bring this up when we were chatting. I feel like it's helpful, to make it clear to the user that if they did have any other devices, they'd be here. Otherwise, I can kind of imagine looking at a listing that only has the current device, assuming this menu only shows the current device, and not being sure whether there are others out there. If you think we're better off without it, I'm fine removing it, though.
  • Checkbox styling: We talked about how Element's standard checkbox looks too much like the green "verified" shield when it's checked, making it kinda weird visually to have those components right next to each other. I still like your idea of restyling the checkboxes here to have a green border, a transparent background, and a green checkmark. Implementing that turns out to be more annoying than I realized, though. I still think it's worth doing, but it may end up being a deeper change that needs more web app team feedback, and I think it can split off into a separate issue and not delay this PR any further.

@amshakal
Copy link

As discussed, let's go with a simple design for 'no other devices'. Attahcing screenshot here.
Screenshot 2021-10-29 at 14 23 23

And spiltting off checkbox design sounds good to me.

Thank you!! :D

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

@duxovni duxovni merged commit d88b8ef into develop Oct 29, 2021
@duxovni duxovni deleted the fayed/device-trust-view branch October 29, 2021 22:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When deleting a session there's no "are you sure?" Implement device trust view in user settings
5 participants