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

Add StatusPageComponent for standard status page layout #6140

Merged
merged 4 commits into from
Apr 4, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 1, 2022

Why:

  • To minimize developer effort in implementing status pages (error, warning, info), and to address currently inconsistencies among them.
  • To address the implementation concerns at Fix countdown JavaScript error for lockout screens #6129 (comment), by (mostly) avoiding inheritance-based presenter object associated with failure screens.

Currently, this merges to #6129, since it implements the "ideal long-term solution" mentioned in #6129 (comment).

Future work would refactor to replace use of app/views/idv/shared/_error.html to use this component instead, as they serve largely the same purpose.

Testing:

  1. Visit http://localhost:3000/sign_up/cancel
  2. Try signing in while locked out
    • Easiest method is via rails c console: User.find_with_email('me@example.com').update!(second_factor_locked_at: Time.zone.now + 10.minutes)

Screenshots:

(Note: There are not expected to be any visual differences with these changes)

Screen Screenshot
Sign Up Cancel image
MFA Lockout image

renders_many :action_buttons, ->(**button_options) do
ButtonComponent.new(**button_options, big: true, wide: true)
end
renders_one :troubleshooting_options
Copy link
Member Author

@aduth aduth Apr 1, 2022

Choose a reason for hiding this comment

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

Future work should adapt app/views/shared/_troubleshooting_options.html.erb into a component, so this could be implemented as a pass-through similar to PageHeadingComponent and ButtonComponent above.

Suggested change
renders_one :troubleshooting_options
renders_one :troubleshooting_options, ::TroubleshootingOptionsComponent

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -1,32 +1,13 @@
class CancellationPresenter < FailurePresenter
Copy link
Contributor

Choose a reason for hiding this comment

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

yesss down with inheritance!

Comment on lines +10 to +15
<ul class="usa-list">
<li><%= t('users.delete.bullet_1', app_name: APP_NAME) %></li>
<li><%= t('users.delete.bullet_2_loa1') %></li>
<li><%= t('users.delete.bullet_3', app_name: APP_NAME) %></li>
<li><%= t('users.delete.bullet_4', app_name: APP_NAME) %></li>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

should we consider refactoring to an array of translations while we're here? One bullet would get extra APP_NAME but that would be ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I like the idea. It's made complicated by the fact that these texts are reused for the "Delete Account" flow, where the second bullet point can have some variability.

<li class="margin-bottom-1"><%= t('users.delete.bullet_1', app_name: APP_NAME) %></li>
<li class="margin-bottom-1"><%= current_user.decorate.delete_account_bullet_key %></li>
<li class="margin-bottom-1"><%= t('users.delete.bullet_3', app_name: APP_NAME) %></li>
<li class="margin-bottom-1"><%= t('users.delete.bullet_4', app_name: APP_NAME) %></li>

I'm not sure how best to handle that, or if it's worth the trouble to refactor it. I'll probably leave it out of this pull request though.

Base automatically changed from aduth-replace-countdown-ref to main April 4, 2022 12:23
**Why**: To minimize developer effort and inconsistencies between status page implementations.

changelog: Improvements, Accessibility, Add meaningful alternative text for failure error icon
Lots of lingering references to it, most of which will likely need to be ported to use StatusPageComponent
@aduth aduth force-pushed the aduth-status-page-component branch from 2d0b1c9 to 3a4b7e8 Compare April 4, 2022 12:24
@aduth aduth merged commit 3d9d08c into main Apr 4, 2022
@aduth aduth deleted the aduth-status-page-component branch April 4, 2022 12:42
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.

2 participants