-
Notifications
You must be signed in to change notification settings - Fork 12
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
A11y/fix your profile layout #1684
Conversation
🧪 Review environmenthttps://2323oazuhfb6hlb75x2nn3rgsq0kvpzb.lambda-url.ca-central-1.on.aws/ |
… go nowhere anyway
…-snc/notification-admin into a11y/fix-your-profile-layout
@@ -175,6 +175,7 @@ jobs: | |||
CYPRESS_BASE_URL: ${{needs.deploy-test-admin.outputs.LAMBDA_URL}} | |||
with: | |||
record: false | |||
config: "video=false,screenshotOnRunFailure=false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with this PR but I slipped it in anyway. It makes the cypress tests run much faster by disabling videos and screenshots, which weren't actually working anyway. In a future PR we could make the screenshots and recordings available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
One suggestion that might be out of scope for this PR is changing the way we display the phone numbers on this page. Without dashes between the phone number parts screen readers will read 6131234567
as six billion one hundred thirty one million....
. With dashes it is read more naturally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes to "wrap" this up:
- Lets allow the "Change" link to wrap on the smallest breakpoints. I'd say let's use something like
flex-wrap sm:flex-no-wrap
. This will give a chance for more content to fit on the smallest screen, and follows a similar pattern we have on the team management screen. - Lets also keep the base font size. The small grey and blue text can be hard to read.
Some design doc for the final layout in Figma
All set! Please re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Summary | Résumé
This PR updates the user profile page to remove the table layout and use flexbox instead.
It also adds 2 a11y features to make navigating this page a little easier:
Before
After