-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor: agency selection frontend #1206
Conversation
This is still a work-in-progress due to some minor questions / fixes needed, but here are some screenshots of it so far: Screenshots
GIFs
Questions for @srhhnry:
|
4894ec0
to
d0b5892
Compare
Thoughts:
Possible to move the box/header/CTA up? Expected behavior would be the user can see the CTA button without having to scroll. |
Oh, and I fixed the MST logo on the Mobile board (it was filling the image instead of fitting to the box!) so at the very least it's not cut off anymore. Standing by on logo assets though. |
Currently I have it configured so that if the user clicks outside the modal dialog, it closes. Bootstrap also gives us the option to not do anything in that case (the user would have to click the "X" to close the modal dialog).
Thanks for pointing these out @srhhnry! I'll look into making these changes. |
Yup! That's perfect. Usually more savvy users will click outside the modal whereas less savvy will look for the X. Having both is great. |
10c0be7
to
45e86f9
Compare
add minimally-required CSS needed to be able to see modal content
use Bootstrap classes where possible. some spacing requires more custom CSS.
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.
I'm going through the modal user experience with an eye on accessibility (keyboard only, keyboard shortcuts) and matching the mocks.
These flows all work:
- Use tab to get focus ring on CTA. Then, use enter or space to open the modal. ✅
- Once modal is open, and nothing is in focus, use escape to exit modal. ✅
- Once modal is open, press tab. The first icon in focus is the X. Use either space or enter on the focused X to exit modal. ✅
- Once modal is open, press tab twice. The MST button is selected. Use either space or enter get to MST (or SacRT). ✅
The one thing missing: There needs to be a focus ring around the whole button very clearly. We don't have designs for it necessary @srhhnry but we can use default (from our styles)
or @srhhnry can design something custom.
Looking more deeply into the way the card link is constructed, it might be tricky (but not impossible) to style the "invisble" linke to not look like below on focus, but rather a ring that goes around the entire card.
Resources:
https://www.sarasoueidan.com/blog/focus-indicators/#new-focus-indicator-accessibility-requirements-in-wcag-2.2
https://www.smashingmagazine.com/2021/03/complete-guide-accessible-front-end-components/#accessible-focus-styles
@angela-tran Thinking about the HTML structure of the MST and SacRT components more, I think it should/could all be a |
height="74" | ||
alt="{% blocktranslate with agency_short_name=agency.short_name %}core.pages.index.agency_selector{{ agency_short_name }}{% endblocktranslate %}"> | ||
<h4 class="card-title mt-lg-3 mb-0 mb-lg-3">{{ agency.long_name }}</h4> | ||
<a href="{{ agency.eligibility_index_url }}" class="stretched-link"></a> |
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.
I wonder if there's a way to move this link or add styles to it so this link does not affect the height/flow of the parent div. Also, I wonder if there's a way to add styles so that the focus ring of this link borders the entire div cleanly.
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.
Similarly, this link needs to at least wrap the h4
text, we shouldn't have a link with no text content.
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.
FWIW @machikoyasuda I think this stretched-link
utility is the right way to do this for cards in Bootstrap: https://getbootstrap.com/docs/4.6/utilities/stretched-link/
But we do need text content in the link
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.
+1 to also needing a solution for the focus state on each card when navigating by keyboard.
c1347be
to
4719906
Compare
I read through that article, and I can see some benefit to switching it to a I will definitely work on fixing the focus style with the suggestion you sent me in Slack @machikoyasuda. |
So @thekaveman and @machikoyasuda , I tried each of your suggestions. The similarities between the two are that
Here's what they each looked like: Use
|
Chrome | Firefox |
---|---|
![]() |
![]() |
Make the element with card
class an <a>
instead of <div>
Code
See 801b57c
Screenshots
Chrome | Firefox |
---|---|
![]() |
![]() |
I went with the latter suggestion because it puts the focus ring around the whole card.
Let me know if there's something I'm missing re: styling the focus ring.
@angela-tran this is looking good! one small thing to add: remove the underline when hovered - by adding custom style for |
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.
Thanks for the changes, this looks good to me.
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.
LGTM! ✨ ✨ 🚀
Catching up on this thread. It sounds like the focus ring can't be designed? And is set by the browser? In case that's not the case, my suggestion would be to keep the behavior fairly close to default, with the exception being the use of our blue (#046b99) for the focus outline. To get real fancy, I'd suggest the hover color (#2f4c65) when selecting the agency button. |
Yeah, so the cards are |
If we'd like to revisit the focus ring styling and using |
Sounds good! |
Closes #1197
Closes #1214
This PR implements the designs from #1090.
The overall approach here was to use Bootstrap's Modal component in a Django template that can be extended. A template called
agency-selector.html
then extendsmodal.html
and handles defining the cards. It is used specifically by thecore/index.html
template.