Skip to content

Better Account Deleted Icon Centering#7037

Merged
eric-gade merged 2 commits intomainfrom
eric-delete-icon-centering
Sep 28, 2022
Merged

Better Account Deleted Icon Centering#7037
eric-gade merged 2 commits intomainfrom
eric-delete-icon-centering

Conversation

@eric-gade
Copy link
Contributor

changelog: Internal, Accessibility, adjusting icon position

🛠 Summary of changes

Updated the alert-icon component sass to include a new convenience class.

Updated the account confirm deleted view to use the new class, which better centers the icon vertically

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Clone locally and setup
  • change the value of email in app/controllers/account_reset/confirm_delete_account_controller.rb to be any string
  • make run
  • Create an account. Then manually navigate to http://localhost:3000/account_reset/confirm_delete_account to see the result

👀 Screenshots

Before:

191615553-b2e5dd0a-aedb-402e-9ff6-8f89e454d1b2

After:

Screen Shot 2022-09-27 at 5 25 28 PM

🚀 Notes for Deployment

Include any special instructions for deployment.

@eric-gade eric-gade requested a review from a team September 27, 2022 21:34
Copy link

@benjaminchait benjaminchait left a comment

Choose a reason for hiding this comment

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

This all looks like CSS changes, I haven't run locally but everything seems reasonable.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it could be a modifier in our convention of using BEM methodology (docs could be clearer, admittedly):

Suggested change
.alert-icon-centered-top {
.alert-icon--centered-top {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I just noticed the login design system link is broken in the Frontend README (it shows https://design.login.gov/components/)

Copy link
Contributor

@aduth aduth Sep 28, 2022

Choose a reason for hiding this comment

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

Also I just noticed the login design system link is broken in the Frontend README (it shows https://design.login.gov/components/)

Good catch! (Though personally that sounds more like an issue with the Design System docs not having a good redirect for that path, since I think it should be a valid URL)

Edit: For posterity, fix for documentation is at 18F/identity-design-system#323 .

eric-gade added 2 commits September 28, 2022 11:49
changelog: Internal, Accessibility, adjusting icon position
changelog: Internal, Styling, updating icon centering
@eric-gade eric-gade force-pushed the eric-delete-icon-centering branch from cc8d8e5 to 8fee34a Compare September 28, 2022 15:49
@eric-gade eric-gade merged commit 0abef7c into main Sep 28, 2022
@eric-gade eric-gade deleted the eric-delete-icon-centering branch September 28, 2022 17:40
@solipet solipet mentioned this pull request Sep 29, 2022
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.

3 participants