-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Footer and signup section UI polish #318
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Removing the visible label and replacing with an arrow icon may cause issues for voice dictation users; the label "Subscribe" may not be easily discerned as a result.
Have we considered other labels?
cc @gretchen-willenberg
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.
The button and button text is now an arrow. My very light research had negative findings on that:
Hopefully there is other, positive research.
Was the mail/envelope icon discussed instead of an arrow? There are a few examples of that and maybe that’s 10% clearer, but still less clear than a button with 1-3 words, e.g. was
Subscribe
but accessibility concerns that this may not stand out.And finally to the copy to help end-user customers subscribe:
Can we add an explicit label on hover and/or aria label?
Click to subscribe to emails
Click
, at least internally: Example: These emails are automatically sent out to either you or the customer. Click on the email templates below to edit.Select
to cover more actionsThere 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.
"Click to subscribe to emails" via aria-label makes sense to me. @wiktoriaswiecicka
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.
Please don't do this. This would add redundant content and be less obvious than what's currently there. It's like adding "Image of" to image alt text.
Let's keep as "Subscribe" as this is the end result of the user action.
Side thought, it seems like visual affordances are being removed with this PR. Previously, the "Subscribe" button looked like a button, with a clear text label. The icon-only control with no explicit border make this UI more difficult to perceive. To some it may look like an "Email" heading with an icon arrow inside a box, not like a form.
Is it too late to revisit this and include the visual affordances that help make this UI more obvious for folks with cognitive impairments or the aging community?
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.
Worth noting, this pattern has also been implemented on at least the search page https://screenshot.click/09-18-cgim2-v95j9.png, but it does use a more informative icon than an arrow. Is a mail/envelope icon a reasonable enough solution here @svinkle or is the lack of a visible button label a dealbreaker in general?
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.
@kmeleta There could be issues here, too. I'm curious if this pattern has gone through usability testing, or if there are plans for this? I don't want my comment to be a bottleneck for this PR, so let's leave as-is for now.
@emma-boardman Are we in a good spot to revisit testing with Fable? I believe last time we chatted about include more than screen reader users.
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'd be delighted to moderate if the Dawn team are interested in doing another round 👍