-
Notifications
You must be signed in to change notification settings - Fork 21
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
Use more accessible visual elements instead of buttons #231
base: main
Are you sure you want to change the base?
Use more accessible visual elements instead of buttons #231
Conversation
Thanks for working on this ! |
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 love seeing an accessibility-related pull request! 🌻
So I couldn't resist leaving a few comments, even though this is still in draft mode. 😄
'<button class="try_examples_button" ' | ||
f"onclick=\"window.tryExamplesHideIframe('{examples_div_id}'," | ||
f"'{iframe_parent_div_id}')\">" | ||
"Go Back</button>" |
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 see no reason to change this element. It really is a button; it's not link, it's not taking you to a new URL.
Generally speaking, you should only use the role
attribute as a very last resort, in cases where you have constraints that prevent you from using the HTML tag that has the role you need. (For example, imagine you want to fix the semantics of the markup output by a tool, such as Sphinx, that's been around a long time. Imagine that there are a lot of downstream styles and code that have been built around that specific markup structure. Then you cannot safely change an a
-tag to a button
-tag without breaking a lot of downstream projects. So if you wanted to fix the semantics, your only safe option would be to use the role
attribute.)
text-decoration: none; | ||
display: inline-block; | ||
cursor: pointer; |
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.
Why is this CSS needed?
f"onclick=\"window.open('{self.lab_src}')\">" | ||
f"{self.button_text}</button>" | ||
f'<a href="{self.lab_src}" ' | ||
'target="_blank" rel="noopener noreferrer" ' |
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 not entirely sure why you need noopener
or noreferrer
.
Generally speaking noreferrer
is used if you're building something like a discussion forum. It removes the incentive for people to post spam links. Noreferrer says to Google something like: as the webmaster of this site, I disavow this link, it could be spam, please don't penalize my search engine rankings for including it on my site.
The general idea behind both noopener
and noreferrer
is that you don't know where you're linking to. You're linking to third-party content that you're concerned could potentially abuse your site or visitor via the link. As far as I understand, that's not the same case here because the try_examples link is created from content that you own and produce.
In any case, this is a change that should be in a separate pull request because it's not really relevant to the other changes in this pull request, and it changes the existing behavior.
f"{self.button_text}</button>" | ||
f'<a href="{self.lab_src}" ' | ||
'target="_blank" rel="noopener noreferrer" ' | ||
'class="try_examples_button" style="text-decoration: none; display: inline-block; color: inherit;">' |
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.
Aren't these styles duplicates of the stylesheet? I think styling them via a.try_examples_button
should be sufficient.
This PR intends to close #226, and is currently a work in progress. I shall update the description soon once I have the buttons working the same as they are outside this PR.