Skip to content

fix(skeleton-text): animation no longer jumps on large skeleton text elements #22697

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

Merged
merged 4 commits into from
May 24, 2021

Conversation

danielehrhardt
Copy link
Contributor

@danielehrhardt danielehrhardt commented Dec 18, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • [ X] Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • [ X] Build (npm run build) was run locally and any changes were pushed
  • [X ] Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • [X ] Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

#22694

Issue Number: #22694

What is the new behavior?

  • Smooth Animation even if the width is more then 800px

Does this introduce a breaking change?

  • Yes
  • [X ] No

Other information

Sorry, something went wrong.

@github-actions github-actions bot added the package: core @ionic/core package label Dec 18, 2020
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Two things I noticed:

  1. The animation should be 1s, not 2s.
  2. The "shimmer" seems to be larger than it was before.

I attached a comparison below (you may need to open the GIFs in a new tab to see the full size). The left GIF has your changes, and the right GIF is what is currently in master.

branch master
NEW OLD

$skeleton-text-background-animated 60%,
transparent
);
animation: shimmer 2s infinite;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
animation: shimmer 2s infinite;
animation: shimmer 1s infinite;

The old animation was 1s long

@danielehrhardt
Copy link
Contributor Author

I changed the animation speed.
I also changed the background gradient. But the old one somehow look wired.
I attached two GIFs.
Maybe you could ask your Team, what you prefer, send me a quick comment which one you want, so i can update the PR.
1.
captured
2.
captured (1)

@danielehrhardt
Copy link
Contributor Author

@liamdebeasi ??

@danielehrhardt
Copy link
Contributor Author

@liamdebeasi Could you please check my last Question?

@liamdebeasi
Copy link
Contributor

@liamdebeasi Could you please check my last Question?

Sorry, most of the team was out the past few weeks for winter holidays, so I didn't see your question.

Ideally this PR would keep the visual effect of the gradient exactly the same as shown in the master column in #22697 (review). Is there any reason why we can't keep the old linear-gradient we had set?

@danielehrhardt
Copy link
Contributor Author

I updated the styles back to the default one.
If something is still missing let me know.

@liamdebeasi
Copy link
Contributor

Thanks, I took a look and some of the issues we discussed previously still persist (mainly the GIFs in #22697 (review)).

I think the solution here is simpler than originally expected. The background-size of the skeleton text is set to 800px, but the shimmer animation moves the background a total of 936px (468 * 2). Changing the shimmer animation to go from -400px to 400px seems to fix the animation on my end.

Can you give that a shot and let me know if it looks good to you? So the animation should look something like this:

@keyframes shimmer {
  0% {
    background-position: -400px 0
  }

  100% {
    background-position: 400px 0
  }
}

If it looks good then I think we can revert the changes made here and just include the CSS Animation change.

@liamdebeasi liamdebeasi changed the title fix(skeleton-text): #22694 animation flickering fix(skeleton-text): animation no longer jumps on large skeleton text elements May 13, 2021
@danielehrhardt
Copy link
Contributor Author

danielehrhardt commented May 13, 2021

Thanks, I took a look and some of the issues we discussed previously still persist (mainly the GIFs in #22697 (review)).

I think the solution here is simpler than originally expected. The background-size of the skeleton text is set to 800px, but the shimmer animation moves the background a total of 936px (468 * 2). Changing the shimmer animation to go from -400px to 400px seems to fix the animation on my end.

Can you give that a shot and let me know if it looks good to you? So the animation should look something like this:

@keyframes shimmer {
  0% {
    background-position: -400px 0
  }

  100% {
    background-position: 400px 0
  }
}

If it looks good then I think we can revert the changes made here and just include the CSS Animation change.

In general on lager Screens the Shimmer appears two times because of the fixed Size of 800px. But it looks good to me.
Should i change the PR or do you edit it directly?

image

@liamdebeasi
Copy link
Contributor

In general on lager Screens the Shimmer appears two times because of the fixed Size of 800px.

I think this is intentional, but it might be worth revisiting the designs for this in the future.

But it looks good to me. Should i change the PR or do you edit it directly?

If you could make the change that would be great. I do not have write access to this PR.

@danielehrhardt
Copy link
Contributor Author

In general on lager Screens the Shimmer appears two times because of the fixed Size of 800px.

I think this is intentional, but it might be worth revisiting the designs for this in the future.

But it looks good to me. Should i change the PR or do you edit it directly?

If you could make the change that would be great. I do not have write access to this PR.

Changed the Code Back to Original with the 400px change.

@danielehrhardt
Copy link
Contributor Author

@liamdebeasi Could you check and merge?

@liamdebeasi liamdebeasi merged commit 1a36922 into ionic-team:master May 24, 2021
@liamdebeasi
Copy link
Contributor

Merged. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants