Account page redesign (LG-3296)#4169
Conversation
mitchellhenke
force-pushed
the
mitchellhenke/account-page-lg-3296
branch
from
3ce18ce to
94c3298
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Just a few comments in passing.
mitchellhenke
force-pushed
the
mitchellhenke/account-page-lg-3296
branch
from
94c3298 to
3bfca12
Compare
mitchellhenke
added
status - ready for review
and removed
status - do not merge
status - work in progress
labels
mitchellhenke
requested review from
aduth,
bpdesigns,
juliasolorzano and
zachmargolis
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM! I ran out of things to nitpick on. Thanks for this, it's epic!
mitchellhenke
force-pushed
the
mitchellhenke/account-page-lg-3296
branch
from
6261c88 to
5c706f3
Compare
bpdesigns
left a comment
bpdesigns
left a comment
There was a problem hiding this comment.
Looks good so long as we can update the third circle in the illustration in the next round to tuck the bottom corners of the screen into it.
mitchellhenke
force-pushed
the
mitchellhenke/account-page-lg-3296
branch
from
5c706f3 to
f4a926d
Compare
| <%= render 'accounts/mobile_nav' %> | ||
| <%= render template: 'layouts/base', locals: { disable_card: true } %> |
There was a problem hiding this comment.
I was observing some strange issues with the session timeout "Need more time?" modal not appearing correctly on the account dashboard (screenshot). After some debugging, I noticed that with the arrangement of this markup, the source code of the page will output the mobile navigation markup outside (before) the <html> (screenshot). I think the problem the issues with the modal are due to the fact that the doctype isn't being interpreted correctly by the browser.
I'm not sure what's the simplest way to achieve it, but I'd expect the markup of accounts/mobile_nav to be rendered within the body tag.
There was a problem hiding this comment.
😱 looking into this
| @@ -0,0 +1,33 @@ | |||
| <h1 class="mt0"> | |||
There was a problem hiding this comment.
I've noticed that a few of these new pages lack a distinct page title describing its purpose ("Two-factor authentication", "History", maybe others).
Related: https://www.w3.org/WAI/WCAG21/Understanding/page-titled.html
Should be relatively straight-forward to include here, using the same labels as in the navigation:
| <h1 class="mt0"> | |
| <% title t('account.navigation.two_factor_authentication') %> | |
| <h1 class="mt0"> |
There was a problem hiding this comment.
For context, I'd stumbled on this in discovering some other, preexisting issues with page titles.
See LG-3502 for full details.
There was a problem hiding this comment.
good catch, thank you. I added a title to all of the new account pages in 5006dce
| <% @view_model.connected_apps.each do |identity| %> | ||
| <%= render identity.connected_app_partial, identity: identity %> | ||
| <% end %> |
There was a problem hiding this comment.
From what I can tell, it was this way as well previously, but I think in the case that the user has no connected accounts, we could be doing better to indicate this on the page:
I can create a ticket if you think it makes sense.
There was a problem hiding this comment.
yeah, it was
I think it would be easier enough to do a lot better. I'm hesitant to put any language at this point, but is it worth not showing the white box at least?
|
Yeah, I think its helpful to show the white box instead of nothing at all.
Gives some indication that if you had accounts they would be here.
*@aduth *would
be interested in your suggestions. Either here or in a JIRA ticket.
…On Thu, Sep 17, 2020 at 10:32 AM Mitchell Henke ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/views/accounts/connected_accounts/show.html.erb
<#4169 (comment)>:
> + <% @view_model.connected_apps.each do |identity| %>
+ <%= render identity.connected_app_partial, identity: identity %>
+ <% end %>
yeah, it was
I think it would be easier enough to do a lot better. I'm hesitant to put
any language at this point, but is it worth not showing the white box at
least?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#4169 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADU6O5DLTU7GI6IDF4HT6NDSGIMYNANCNFSM4QZJTBLQ>
.
--
Ben Peterson
Experience Designer & Researcher
Pronouns: He/His
Timezone: EDT
GSA <http://www.gsa.gov/portal/category/100000> / TTS
<https://www.gsa.gov/about-us/organization/federal-acquisition-service/technology-transformation-services>
/
18F <https://18f.gsa.gov/>
|
|
@bpdesigns Yeah, even just a simple text "There are currently no connected accounts to show" would suffice, I think. Though I do like the idea of being even more helpful in what you suggest as an addendum of "...they would be shown here if there were any". I created a ticket at LG-3504 to track this. |
|
Looks great!
…On Thu, Sep 17, 2020 at 11:01 AM Andrew Duthie ***@***.***> wrote:
@bpdesigns <https://github.com/bpdesigns> Yeah, even just a simple text
"There are currently no connected accounts to show" would suffice, I think.
Though I do like the idea of being even more helpful in what you suggest as
an addendum of "...they would be shown here if there were any".
I created a ticket at LG-3504 to track this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4169 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADU6O5H6E7BBYIDCSXDKEXDSGIQARANCNFSM4QZJTBLQ>
.
--
Ben Peterson
Experience Designer & Researcher
Pronouns: He/His
Timezone: EDT
GSA <http://www.gsa.gov/portal/category/100000> / TTS
<https://www.gsa.gov/about-us/organization/federal-acquisition-service/technology-transformation-services>
/
18F <https://18f.gsa.gov/>
|
5 participants
My personal sandbox is deployed with the IDP on this branch as of 3a07ef8