Skip to content
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

Adding a content block loading animation. #1730

Merged
merged 11 commits into from
Mar 22, 2019

Conversation

daveyholler
Copy link
Contributor

@daveyholler daveyholler commented Mar 13, 2019

Summary

Creating a simple loading animation for content blocks. I'm open to ideas on this. I wanted to start with something dead simple and get some eyes on it.

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@daveyholler daveyholler force-pushed the content_loading_animation branch from 22a23a3 to 748681e Compare March 15, 2019 21:33
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool and would be nice to start using. I have a couple suggestions around color and timing that was easier to just play around with locally so I can send you a PR with my suggestions.

Also, since this is representing blocks of text, it would be nice if it was also sized like our blocks of text. For instance, here is or normal paragraph text overlayed on the current size:

And if instead, the lines were only 16px tall and totalling a line-height of 24px, it could look like:

Allowing any fade-in of content to take up about the same amount of space (especially for single lines).

And then maybe there's a size prop like EuiText has that will automatically adjust the whole SVG size to match the sizing of the text and line-heights of EuiText sizes.


Speaking of transitioning between this loading and the real content, how do we propose making that easy for the consumer. Right now it seems that they will have to do a bunch of absolute positioning? Or is there a better way?

src-docs/src/views/loading/loading_example.js Outdated Show resolved Hide resolved
src-docs/src/views/loading/loading_example.js Outdated Show resolved Hide resolved
src/components/loading/loading_content.tsx Outdated Show resolved Hide resolved
src/components/loading/loading_content.tsx Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Mar 18, 2019

Also, we should have someone more knowledgable than me to take a look at the performance hit this may cause. When adding 5 of them on one page, the browser makes my computers fans spin up.

@chandlerprall
Copy link
Contributor

When adding 5 of them on one page, the browser makes my computers fans spin up.

I modified the example to include 5 loaders and saw ~10% CPU usage and ~50% GPU usage compared to barely any of either when I removed all instances of the new loader. The CPU usage is caused by a layout invalidation, and GPU is re-painting a lot of things as a result. Removing the SVG animate tag "fixes" the issue. To be performant, the animation should be converted to a CSS transform/animation.

https://www.crmarsh.com/svg-performance/

@daveyholler
Copy link
Contributor Author

@chandlerprall Thanks for the investigation. I'll work on getting those animations converted to css.

@daveyholler daveyholler force-pushed the content_loading_animation branch 2 times, most recently from bcc26ab to 5eaffc5 Compare March 20, 2019 17:58
@daveyholler daveyholler changed the title [WIP] Adding a content block loading animation. Adding a content block loading animation. Mar 20, 2019
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a suggestion, I played around a bit with the color gradient and animation settings to make the gradient move more discretely from left-to-right. As it stands, it felt as though it was pulsing (fading in and out) versus progressing.

Preview (the gif is choppier than the real thing)

loading-content

To accomplish this, I removed a couple of the stops, narrowed down the remaining stops, decreased the background size, and reversed the gradient so that a dark section moves across on a lighter placeholder background color.

Here is the code I ended up with in _loading_content.scss if you'd like to try it out:

$gradientStartStop: shadeOrTint($euiColorLightestShade, 4%, 4%);
$gradientMiddle: shadeOrTint($euiColorLightestShade, 8%, 8%);

.euiLoadingContent__loader {
  width: 100%;
}

.euiLoadingContent__single-line {
  height: $euiSize;
  margin-bottom: $euiSizeS;
  border-radius: $euiBorderRadius;
  width: 100%;
  background-size: 200% 100% !important; // sass-lint:disable-line no-important
  background: linear-gradient(
    to right,
    $gradientStartStop 8%,
    $gradientMiddle 18%,
    $gradientStartStop 33%,
  );
  animation: GradientLoad 1.5s $euiAnimSlightResistance infinite;

  &:last-child:not(:only-child) {
    width: 75%;
  }
}

@keyframes GradientLoad {
  0% {
    background-position: 100% 0%;
  }

  100% {
    background-position: -100% 0%;
  }
}

@daveyholler daveyholler force-pushed the content_loading_animation branch from 876ec55 to 077d824 Compare March 20, 2019 23:29
@daveyholler
Copy link
Contributor Author

@ryankeairns Thanks for the feedback. Your version of the animation is way smoother. I stayed with the light-on-dark animation though, cause the goal was to make it look like a shimmer. I'm totally open to other opinions though.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple suggestions mostly around naming.

src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
import classNames from 'classnames';
import { CommonProps } from '../common';

type LineRange = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary now that each line is a block level element so you don't have to calculate the y position. You can leave lines as a number type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I limited this more as a design decision to prevent it being used for giant walls of text. That's just a philosophy thing though. I can remove the limit if the preference is to keep EUI less opinionated. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with being opinionated, I was just thinking about cleanliness. I'm fine if you'd like to keep it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool beans. I'll leave it here for the time being. We'll see what kind of feedback we get.

src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
@daveyholler daveyholler force-pushed the content_loading_animation branch from 077d824 to 21ff5a6 Compare March 21, 2019 16:47
@chandlerprall
Copy link
Contributor

Tested performance after the latest upgrades, it's better but not by a lot. Unfortunately, animating background-position is still triggering layout changes. If we want to improve this, I think the only way would be to put the gradients in a container and animate their transform: translateX() value to scroll across the container.

It may not be that big of a deal. I filled my screen with the loaders and my system still had extra processing time. If the loaders are used correctly, this should be fine.

@daveyholler
Copy link
Contributor Author

@chandlerprall I can definitely take another go at this to be more performant. Or I can provide some extra documentation about best practices?

@chandlerprall
Copy link
Contributor

Either/or, in my opinion. Would be nice to have it be truly performant, but if that's too much effort for this simple, not-oft-used component then some short documentation around the fact would be fine!

@daveyholler
Copy link
Contributor Author

@chandlerprall Ok. I went ahead and switched to animating on translateX instead of the background position. Hopefully this shows some performance increases 😹

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daveyholler it's a thing of beauty, thank you for the changes!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check this in IE and FF?

Just had a nit about class naming.

src/components/loading/loading_content.tsx Outdated Show resolved Hide resolved
src/components/loading/_loading_content.scss Outdated Show resolved Hide resolved
@daveyholler daveyholler force-pushed the content_loading_animation branch from 9c8bc32 to cf28ba7 Compare March 22, 2019 18:56
@daveyholler daveyholler requested a review from cchaos March 22, 2019 19:12
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being a stickler, but there's still an unchecked item for Jest tests. Even though these components are purely visual, they should still have tests so that we make sure no one breaks it unintentionally in the future.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! Thank you!

@daveyholler daveyholler merged commit 4bdad39 into elastic:master Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants