The Security tab doesn't have fields, but uses fieldsets. Switching to using heading tags will look a bit cleaner.

See point 8 at #2239973-51: Deploy two-factor-authentication on drupal.org for the original suggestion.

CommentFileSizeAuthor
#8 2480577-tfa-basic-setup-ui-8.patch2.09 KBcoltrane
#8 tfa-account-disabled.jpg159.82 KBcoltrane
#8 tfa-account-enabled.jpg213.48 KBcoltrane
#3 2480577-tfa-basic-setup-ui-3.patch6.82 KBcoltrane
#2 tfa-disable-location.png93.49 KBcoltrane
#1 Screen Shot 2015-04-29 at 5.06.50 PM.png60.22 KBdrumm
#1 2480577.diff5.49 KBdrumm

Comments

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Active » Needs review
StatusFileSize
new 2480577.diff5.49 KB
new Screen Shot 2015-04-29 at 5.06.50 PM.png60.22 KB

Here is a patch for review. Also, '#theme' => 'link' is used so links can be altered, such as adding classes.

With Drupal.org's theme, it looks like this:
Screenshot

coltrane’s picture

coltrane
he/him
commented
Issue summary: View changes
StatusFileSize
new tfa-disable-location.png93.49 KB

Thank you @drumm! One visual issue is the lack of separation before "Disable TFA". How about if it was moved up next to the status indicator to appear after the set time?

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2480577-tfa-basic-setup-ui-3.patch6.82 KB

And as patch. All tests pass.

I don't love moving up an action that shouldn't happen very often, but it's still out of the way and is simple change.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Reviewed & tested by the community

Looks good.

  • coltrane committed 627287c on 7.x-1.x authored by drumm 
    Issue #2480577 by drumm: Improve UI for main Security tab
coltrane’s picture

coltrane
he/him
commented
Status: Reviewed & tested by the community » Fixed
Bojhan’s picture

Bojhan commented
Status: Fixed » Needs work

I see a couple usability improvements:

  • Lets not abbreviate Two-factor authentication, its not a common term so likely more scannable by using the actual words
  • Lets have a heading status - with a text "Enabled" below it not next to it. I don't think you need to repeat "TFA" here" - or can you do anything else on this page?
  • We have no summaries for the browsers, applications - when they are setup can we give an indication? (e.g. "2 browsers configured")

I'd like to just make those improvements in HMTL to show, but I cant really figure out where to go. Can I view this online somewhere?

coltrane’s picture

coltrane
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new tfa-account-enabled.jpg213.48 KB
new tfa-account-disabled.jpg159.82 KB
new 2480577-tfa-basic-setup-ui-8.patch2.09 KB

@Bojhan thanks for your feedback.

I can reuse the format of the individual plugins for the status for the status and set the current state (enabled or disabled) as the header:

Here's the enabled state which also shows browser summaries.

The opening page text defines TFA and I think a lot of people are familiar with the term now.

As far as live testing, https://www.drupal.org/node/2239973 links to a devdrupal site.

drumm’s picture

drumm
he/him
Location NY, US
commented

I think the 'Enabled'/'Disabled' heading might need some more context, I expect many people will skip the paragraph above.

Would it make sense to merge with the 'TFA application' section? As in the section would be:

TFA disabled

Generate verification codes from a mobile or desktop application. Settings last changed !time.

Set up application

or when enabled:

TFA enabled

Generate verification codes from a mobile or desktop application. Settings last changed !time.

Reset application

Disable

For now, this patch is applied to the dev site referenced in #2239973: Deploy two-factor-authentication on drupal.org.

drumm’s picture

drumm
he/him
Location NY, US
commented
Assigned: drumm » Unassigned
coltrane’s picture

coltrane
he/him
commented
Status: Needs review » Needs work

I like that suggestion @drumm. Setting to CNW to make those changes.

banviktor’s picture

banviktor commented

I think we should keep #2478307: Allow user to decide which method is primary in mind. If we change the main plugin's title to TFA en/disabled it might get messy.