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

Event transitions 🎉 #532

Merged
merged 5 commits into from
Dec 27, 2022
Merged

Event transitions 🎉 #532

merged 5 commits into from
Dec 27, 2022

Conversation

JustinStitt
Copy link
Collaborator

Demo

show1

@vercel
Copy link

vercel bot commented Sep 7, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
acm-csuf-site ✅ Ready (Inspect) Visit Preview Oct 15, 2022 at 8:32AM (UTC)

@diamondburned
Copy link
Collaborator

As a side note, the Algo event's description escapes the <p> tag, probably because nested <p> tags are invalid. It should probably be in a <div> instead. Note the HTML_TAG_START and HTML_TAG_END comments.

cc @EthanThatOneKid

image

@diamondburned
Copy link
Collaborator

Also, can we squash these commits?

@vercel vercel bot temporarily deployed to Preview September 7, 2022 19:59 Inactive
@JustinStitt
Copy link
Collaborator Author

Also, can we squash these commits?

I like have PR revision history. Perhaps maintainer can squash pre-merge?

@JustinStitt
Copy link
Collaborator Author

Also, this package may be of interest. It does detail-summary animations in-house.

Adds a bit of css bloat but simplifies the DOM.

Copy link
Collaborator Author

@JustinStitt JustinStitt 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 the power to LGTM but won't :)

@diamondburned
Copy link
Collaborator

As a side note, the Algo event's description escapes the <p> tag, probably because nested <p> tags are invalid. It should probably be in a <div> instead. Note the HTML_TAG_START and HTML_TAG_END comments.

After looking into it, this doesn't happen with JS enabled. It might be the server-side rendering attempting to sanitize the HTML differently compared to the browser.

@jaasonw
Copy link
Contributor

jaasonw commented Sep 9, 2022

am i missing something or it seem kinda janky to me?

@JustinStitt
Copy link
Collaborator Author

am i missing something or it seem kinda janky to me?

The behavior or my implementation?

@jaasonw
Copy link
Contributor

jaasonw commented Sep 9, 2022

the behavior

@jaasonw
Copy link
Contributor

jaasonw commented Sep 9, 2022

https://user-images.githubusercontent.com/8981287/189256563-8a2cc082-ccc6-40f3-acef-154185aa3c03.mp4
like it just kinda snaps down and when u minimize it kinda just disappears

@JustinStitt
Copy link
Collaborator Author

https://user-images.githubusercontent.com/8981287/189256563-8a2cc082-ccc6-40f3-acef-154185aa3c03.mp4 like it just kinda snaps down and when u minimize it kinda just disappears

hmm, idk. I just tried to be clever and use the built-in Svelte transitions. It does look weird to me as well.

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

I appreciate the changes in this PR because @JustinStitt was able to create an enhancement that gets added to the page progressively when JavaScript is enabled. This means that he took into account backwards-compatibility, allowing the component to properly function even when the expected JavaScript code is absent.

@vercel vercel bot temporarily deployed to Preview September 18, 2022 06:08 Inactive
@EthanThatOneKid EthanThatOneKid enabled auto-merge (squash) October 4, 2022 22:22
@vercel vercel bot temporarily deployed to Preview October 4, 2022 22:31 Inactive
@vercel vercel bot temporarily deployed to Preview October 5, 2022 15:41 Inactive
@jaasonw jaasonw disabled auto-merge October 5, 2022 19:57
@jaasonw
Copy link
Contributor

jaasonw commented Oct 5, 2022

wait hold up, am i the only one that finds this initial jerk kinda weird

Screen.Recording.2022-10-05.at.12.58.00.PM.mov

@JustinStitt
Copy link
Collaborator Author

JustinStitt commented Oct 5, 2022

I am not seeing an initial jerk. It's animation timing is probably a Cubic Bezier with a moderately high starting slope but that is just part of the animation.

This is preferred to a slow, unresponsive, and less punchy ease-in timing imo.

@diamondburned
Copy link
Collaborator

I am not seeing an initial jerk.

Probably because the animation timing is very low. By doing this change:

--- a/src/routes/events/event.svelte
+++ b/src/routes/events/event.svelte
@@ -93,7 +94,7 @@
     </noscript>
 
     {#if shown}
-      <p class="event-description" transition:slide>
+      <p class="event-description" transition:slide={{ duration: 2000 }}>
         {@html info.description}
       </p>
     {/if}

You'll see that the action buttons inside each event are not actually transitioned properly:

Screencast.from.05-10-22.15.24.01.webm

This probably explains the initial jerk.

@jaasonw
Copy link
Contributor

jaasonw commented Oct 5, 2022

oh, good catch, yea these buttons needa be transitioned as well as the description
image

@vercel vercel bot temporarily deployed to Preview October 15, 2022 08:27 Inactive
@vercel vercel bot temporarily deployed to Preview October 15, 2022 08:30 Inactive
@vercel vercel bot temporarily deployed to Preview October 15, 2022 08:32 Inactive
@jaasonw jaasonw marked this pull request as draft October 18, 2022 03:49
@netlify
Copy link

netlify bot commented Dec 26, 2022

Deploy Preview for acmcsuf failed.

Name Link
🔨 Latest commit 273d42b
🔍 Latest deploy log https://app.netlify.com/sites/acmcsuf/deploys/63aae4bc6f30aa00082de193

@jaasonw
Copy link
Contributor

jaasonw commented Dec 26, 2022

output.mp4

@jaasonw jaasonw marked this pull request as ready for review December 26, 2022 07:42
@EthanThatOneKid EthanThatOneKid enabled auto-merge (squash) December 26, 2022 23:26
@EthanThatOneKid EthanThatOneKid merged commit 90e4735 into main Dec 27, 2022
jaasonw added a commit to jaasonw/acmcsuf.com that referenced this pull request Dec 27, 2022
* Add transitions to event items
* fix event transitions fr

Co-authored-by: Ethan Davidson <31261035+EthanThatOneKid@users.noreply.github.com>
Co-authored-by: jaasonw <jayywong92@gmail.com>
jaasonw added a commit that referenced this pull request Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants