-
Notifications
You must be signed in to change notification settings - Fork 397
Add fullscreen button to examples #2274
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
base: main
Are you sure you want to change the base?
Conversation
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.
Useful helper, reasonable implementation! I think we can omit the project file: this is simple enough to recreate as needed if we can't get in touch with you.
Fair enough. I mostly used the chance to play around with graphite.rs - very cool, if a few rough corners. |
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.
Has one definite blocker in the accessibility department and minor stylistic issues with the icon that are bothering me.
href="https://github.com/bevyengine/bevy/blob/latest/{{ page.extra.github_code_path }}"> | ||
<a class="example__back" href="{% block examples_url %}{% endblock examples_url %}"><i | ||
class="icon icon--chevron-left"></i> Back to examples</a> | ||
<a id="fullscreen-button" class="example__fullscreen"><i class="icon icon--fullscreen"></i></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.
Immediate concern is that this is an improper use of the anchor element. Anchors are meant for navigation to other places, whether they be to files, other web pages, another section in the same page, or even to someones email address. For an action like initializing fullscreen we should be using a button element in an anchor elements stead. Why this matters is accessibility and semantics as text to speech tools will have little context with this anchor that is not an actual anchor, and so any accessibility functions with regards to ARIA and such you get with using the semantic element properly is lost.
- Should use
<button>
and not<a>
- Should have a name to give text to speech readers a description, "Enter fullscreen"
[…] </i>Enter fullscreen</a>
. This can be hidden via CSS (immediate thought is div around text applying the CSS tricks mentioned in the article): https://www.a11yproject.com/posts/how-to-hide-content/ .
See this for more on button accessibility: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/button#accessibility
("github", 24), | ||
("pencil", 19), | ||
("times", 16), | ||
("fullscreen", 24), |
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.
Oh and a third thing that just crossed my mind, we should have the cursor be a pointer over the button to enter full screen. https://developer.mozilla.org/en-US/docs/Web/CSS/cursor |
Sometimes, especially on smaller screens, it can be useful to Fullscreen an example to view it properly. This PR adds a simple Fullscreen button to the examples.
I've deliberately decided against making it hover over the example in a corner, as that would cover parts of the example and potentially interfere with UI placed in the same location.
I've made the icon myself, using https://graphite.rs. Should I also add the project file, as it allows parametric control to adjust the icon?