-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
fix(api): Use internal example in KeyFrameEffect pages #34613
Conversation
Preview URLs
(comment last updated: 2024-07-12 03:27:26) |
94426b1
to
37e80b9
Compare
Sorry, I've had so many reviews dumped on me recently that I'm struggling to keep up with them all. I'm removing myself from a couple of them. |
c1e7891
to
00f15c8
Compare
Hey Onkar, I've added myself as a reviewer. I'll try to take a look today 🙏🏻 |
Co-authored-by: Brian Thomas Smith <[email protected]>
Looking great. I have one concern that we're duplicating the code so next time we make changes, we have to update multiple places, but I'm not sure what the better option would be. Do you have any thoughts about simplifying the KeyframeEffect/target example so it's more minimal? Getting rid of the Otherwise, good to go, IMO |
Unfortunately, all the examples are not the same they have sight variations. Some log the corresponding member/function in the code.
CSS block is not relevant so I've hidden it. Got rid of |
8e70c3d
to
8ae981c
Compare
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.
Thanks a lot! Couple of minor suggestions for you, but I think we're ready to go when you've had a look at my comments 👍🏻
Co-authored-by: Brian Thomas Smith <[email protected]>
Looks great! Merging now! |
As asked for in the issue, the PR adds examples in the KeyFrameEffect pages.
The new example makes 🤣 emojy really roll on the floor. Also,
KeyframeEffect.spacing
doesn't exist in specs or MDN so it's been removed.