-
Notifications
You must be signed in to change notification settings - Fork 4
Add "loading" and "running" animated icons #383
Conversation
… animations to the files
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hashicorp/flight/9TiKQcjvJbghv5KiM2n7UzgBNaHu |
@didoo I see your point that the animation has a purpose. Did you try to slow down the animation? That would still respect a reduced animation preference without removing it entirely. That could be an appropriate compromise and I believe it would still meet accessibility criteria. |
👍
Good idea! Didn't think of it :) Let me try and see if it works |
@MelSumner nope, doesn't work (I suspect because these are SMIL animations, and can't be controlled via CSS). |
Ok, I will do some research. We should not use this approach if we cannot make it respect the prefers-reduced-motion flag, this will be particularly troublesome for folks with attention or vestibular issues. I will do some more digging and see if there is an accessible way to do this. 👍 |
@MelSumner this is the problem, so you have more context. At the moment we have 3 different formats in output:
Now, how do we apply animations to all of them? If we use CSS, we can control the CSS of the That's why I went with the second option, use SMIL/SVG animation: because in this case the animation is "embedded" inside the icons, consumers just need to use the icon and that's it, it works out of the box without them worrying to implement custom animations. Potentially we could decide to split the output, have the SMIL/SVG animation for marketing websites, and CSS animation for product applications, but in this case we add some extra complexity to the generation pipeline, but also would not solve the problem for them. My thinking, as I told you before, is that this is one of the cases in which making an exception makes sense. The reasons I see are these:
PS: I know is a delicate matter, don't take mine as insensibility towards the accessibility issue. |
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! 👍
@didoo let's get @ashleemboyer to weigh in on this, too. Thinking about more options:
Also maybe Ashlee has some other ideas. |
Good idea.
We should consider also the React/SVG component.
Potentially, yes. |
Hey y'all! Thanks for the tag. I'm just getting out of all-day MKO activities and it's after 6pm where I am. I'll write up a response tomorrow morning. 😊 |
Alright, I've gone through the description and comments on this PR, checked out the linked codepen, and have done a little research as well. Ready to give my take on this. 😊 As someone that is negatively impacted by animations on a frequent (but not constant) basis, I have to say that I think it's absolutely necessary to make animations work with As for how we can make this work with I think another topic here is what should happen in the |
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.
After review and getting feedback from other a11y devs, I don't think we can use this approach because it is not accessible.
I am willing to meet to discuss other potential solutions, some of which were shared in this PR's comments.
…ss names (for consistency with other components in HDS, and because we want to use the same class names in the React implementation)
…light-icon” addon
@MelSumner @ashleemboyer as anticipated yesterday to Melanie, I have pushed the last changes to use CSS animations instead of SVG/SMIL animations for the That said, I am still not 100% sure this is what we should to. Both solutions have pros and cons, and I'll try to list them below (from my point of view). Both solutions have been implemented, I can revert to a previous commit in git (SVG/SMIL) or I can leave things as they are now (CSS). But I'll leave to you the last call. Let me know what you think, thanks. SVG/SMIL implementationPros
Cons
CSS implementationPros
Cons
Notice: I am totally aware that choosing SMIL over CSS means favouring the Dev Experience vs the User Experience, but quoting Melanie:
and I think the key word in this case is "pragmatic". My opinion is that in this very specific case, the pragmatic choice is to go with SMIL. But it's my opinion, and in a Cap Watkins scale I am on a 3 out of 10 on this, so as I said above your opinion prevails on mine. PS: @ashleemboyer if you decide to go with the CSS implementation, can you check the documentation for React? I tested locally using DotDev, so the code I added in the snippets is the one that worked there, but maybe I missed something or something is not clearly explained. Thanks |
👋
Not sure what's involved in the SMIL impl. but guessing it would work? I'd prefer to use CSS though.
I just gave this a quick go with some CSS: Guessing if you go with a CSS route we can use it also, but I think I'm easy with whatever you do here as long as I can use an SVG with CSS. P.S. Oh i forgot to say, I would imagine we'll have this as a user setting in our settings page along with theme, so it will work with @media query being set, and/or just the user selecting "reduced motion" from our settings page also. |
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 conditionally approving but I have a few reservations that I'd like us to revisit later:
-
why are we doing something different for Consul? It's an Ember app and should consume the addon in a way that is consistent with the other Ember apps. This is not to call out Consul directly but rather the concept of supporting a non-standard thing when the goal is to improve standardization across the board. So let's bring this up in a team discussion, please.
-
animated icons are small and are also informative, so slowing them down for
prefers-reduced-motion
is better than turning the animation all the way off (in these cases, users are not receiving feedback that the loading is still occurring, since a non-animated state is the same as a frozen state). We should document this information for consistency and so if it comes up later we can discuss with context. -
If consuming teams want to add extra options in their user menus, the CSS is easy enough to change/override. So I don't think we are blocking anyone there.
-
I am still not sure why we are shipping React components (vs providing CSS and allowing them to build their own as they wish), since I understood previously that they would just be consuming the tokens and/or stylesheets and writing their own components, and the design system itself would be shipping components for product use (in Ember).
-
Let's revisit why we needed to include cheerio- we worked really hard to get jQuery out of things and I would like to see us avoid re-introducing.
Again, these are topics that we should discuss in our next team meeting and should not block merging this code.
@MelSumner as far as I'm aware you are not doing something different for Consul, we are fine with what is here. We take a similar approach to Boundary and just use the svg icons. I can't really speak for Boundary but this works for all of us and is something I was very loud about from the outset that we "just wanted some svg icons", nothing overly complicated. Please make sure you include me in your team discussion if one happens. P.S.
|
@johncowen look at this commit, you will see how all the SVGs (SVG standalone, SVG React, SVG sprite) have an
@johncowen of course you can do it, but if in the future we have change an animation parameter (eg the duration), you will have to do the same (and someone will need to remember to do it/tell you); same if we add a new animated icon.
As it is now, for the "targets" supported, will work with the
@MelSumner see JC response. we're not doing something different, we're just taking in account a consumer that is using the SVG standalone icons in a slightly different way than other consumers.
Not sure if you're proposing to add an extra media query (for when the user sets the
No, in theory consumers should not mess with the animation of the icons. @cveigt has done the work of fine-tuning these values (and the icons), I think we should consider (and respect) their decisions.
Two things.
Sorry, but these are two completely different cases. JQuery is for DOM manipulation in a browser. Cheerio is used in a Node.js script run at build time to manipulate the SVG (as XML). Will never end in the users' browser. So why should we not use it?
See the comments above, and for the ones that are not clear happy to discuss with the whole team. |
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.
There's one thing that looks like a typo, and I have a question. Does there need to be two separate animation.css
files generated that are the same? Is it possible to just have one that any usage of icons can import?
As a consumer on the React side, I don't see this as a con. The import only needs to be made one time per project, so once it's in the code there's nothing extra for us to do.
I want to be very clear about why this is such an important topic and why Developer Experience vs. User Experience is an unfair comparison to make in this case. Animations can physically harm many people including myself, which is why I have such a strong opinion. Doing what we can as developers to prevent harm to users is always pragmatic in my opinion.
Docs look good and make sense! |
Hey just catching up here briefly:
Ah perfect! Thanks for pointing me to that 🙇 Yeah, so this would work for us also. But just to be clear, I definitely prefer the CSS approach.
What I did is using all the code you've provided here, i.e. if you change that code we will automatically get the updates also, if/when we decide to If you "add a new animated icon" they automatically get picked up so they are available to us. Whichever way though, somebody probably needs to make us aware they are there so we know we have those available to us to use, either by just communicating that to us, by us seeing them in our designs, or by us finding them by searching the icons docs page. If you add an icon called 'fantastic-icon' I won't just magically know I can write 'fantastic-icon' somebody has to tell me that 'fantastic-icon' is an identifier I can use for icons, either by just telling me, seeing a new icon in a design or searching for it in the docs page and coming across it (unlikely as I should 'find out' via the design). My more overall opinion is, having new icons automatically added to a npm package in my eyes is not a 'feature'. They are 'added' when I see them in a design I'm building. On the other hand, existing icons being tweaked and me not having to know that they have been tweaked that I can opt in/out of via semver is a 'feature'.
Cool, thanks for letting me know, we can add that feature from our side then (the in UI setting to turn it on/off addition to the @media switch) Separately, overall it doesn't seem like its much extra effort to support switchable reduced motion icons, so while I'm here, I'm a big +1 to @ashleemboyer 's comments. |
OK, the matter is settled then: we go with CSS animations. Thanks all @ashleemboyer @MelSumner @johncowen for the comments and feedback, really appreciated. @cveigt can you sync with @MelSumner and @ashleemboyer and come up with a value for the |
Wanted to leave an answer for this question in case it comes up again in a future conversation on animations and
Browsers don't handle disabling animations whether CSS, GIFs, JavaScript, etc. If browsers started automatically disabling animations, countless sites are sure to break. It's also not possible for browsers to detect if JavaScript is being used to animate, so while it would be nice if browsers handled it for us, they just don't and that's what we have to work with. |
I couldn't find any particular recommendation or prescription for when users set I did some testing, and think Because there are several types of vestibular disorders and different levels of each of them, it seems safer to go with a very, very slow animation as long as it is still perceived as a loading/running state within the UI. |
As theres a nice convo going on here on the 'loading' reduced motion thing I had a couple of questions, but first a little background: I vaguely remembered that browsers used to show like a 'barber shop' striped bar when a So my first curiosity question is, if the browser vendors aren't stopping these smaller type of animations should we be? Fully understand if the answer here is simply 'yes' - but then that does make me wonder why browser/OS vendors don't stop the native animation, I wondered if thats a conscious decision or is it because they just didn't get there or something. Does anyone know if there is anything at an OS level outside of the browser that stop animating when the 'Reduce motion' check box is selected in Settings? (btw I am still pro stopping/slowing the animations for reasons given above) Second question is, would it be an idea to swap out the icon for a different one, similar to the old style indeterminate progress bar, maybe a static icon that looks like the barber shop pole thing. To me means 'loading' even though it's static. That ^ to me means 'loading' more than a quarter filled circle, but maybe its just me. Just to be clear I'm still pro doing something (anything) for reduced motion, but I'd like to understand more what the 'done thing' is and why (if folks know). Also wanted to put out there the static barber shop pole thing so see what other insights/thoughts folks here had on that. |
I'll let our accessibility experts chime in, but maybe this helps answer your questions. One of WCAG recommendations that applies specifically to animation is Pause, Stop Hide. This states that one exception for this recommendation is essential animations.
There is a particular example that describes our case:
So that might be the reason or one of the reasons why browsers/OSs are not disabling them. I'd like to try to keep the animation as long as it is feasible from an a11y perspective. As @didoo pointed out, the "animation for this icons is informative and not just decorative". |
Great find @cveigt ! From reading that example, initial thoughts are - it sounds like in some cases keeping an animated loader is fine if:
So it all depends on context. I think we mostly use this with other content on the page, so does that mean we should stop it? Or maybe leave it up to the designer/engineer to decide whether to stop it on a case by case basis? Off the top of my head I can't think of anywhere in our products where we use this particular icon on its own with no other content, but I could be wrong. And also sods/murphys law comes into play here, if we say it will never happen then it surely will.
Whilst I agree with this to some extent, I would argue that it's not completely informative, the animation itself is not informative. It's an indeterminate loader i.e. the progress of the bar around the circle does not mean anything, the animation and icon is decorative. The fact that this signifies "this is loading" is the information it replaces. This information could also be portrayed with text or a static image/icon (as mentioned above). In the same way that the text "Information" could be replaced with a little 'i circle' icon. The icon is decorative, the word "Information" (hidden from vision) is the information that is optionally read out to a screen reader. Using that comparison the word "Loading" (or maybe sometimes "In progress") is the information that would be hidden from vision and replaced with the decorative image/icon. Overall I'd say an image/icon is informative if it has meaningful content, in this case I don't think it does. (opinions are my own etc etc) Again thanks for the links @cveigt, exactly the type of stuff I was after 🙌 gonna read it in more detail. Let me know if you or anyone else has more thoughts 🙏 |
Wow, off on a little tangent, but this is super interesting for me personally
|
(and fixed typo in animation name)
# Conflicts: # flight-icons/catalog.json # flight-icons/svg-sprite/svg-sprite-module.js # website/public/assets/zip/flight-icons-svg.zip
@MelSumner @ashleemboyer @cveigt + @johncowen |
📌 Summary
This resolves #282.
In this PR I have:
src
preprocess
) to include also adding custom SVG animations to the “loading” and “running” files (usingcheerio
for XML/SVG manipulation)flight-icons
to2.1.0
Notice: at the beginning I was thinking of using CSS for the animation; but then when working on the ticket I realized this would have meant that the standalone SVG icons and the React icons consumed by the marketing/dotdev team would not be animated (and require custom animation on the consumers side). So I decided to use add the animation directly in the SVG files, so that every format (standalone SVG, SVG sprite/Ember icon, React icons) would be animated.
@MelSumner I tried to use the
prefers-reduced-animation
media query to disable the SVG animation, but without success. do you know a way to do it? I have also tried to activate the setting in MacOS and check if the browser would disable the SVG animations, but it didn't. My thinking at this point is that because animation for this icons is informative and not just decorative, makes sense to leave it as is (otherwise the complexity of support different formats would be much more). If you feel strongly about it let me know and we can discuss alternatives./cc @hashicorp/design-systems @cveigt @heatherlarsen
👉 Review by files changed
when doing the review, is not possible to view the icons in the preview branch because they're not been released yet as package so not installed during the website build for the preview; you can see them in the videos below
https://user-images.githubusercontent.com/686239/149758296-e627834d-cfde-4557-8f5a-f7daada38c6c.mov
https://user-images.githubusercontent.com/686239/149758301-f9d6a18a-56be-4d9c-9950-3a03d6729b87.mov