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

Add slide-in animation for pending updates notification #6272

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Mar 20, 2024

Part of #6255

Add a slide-from-right + fade-in animation to the pending updates notification.

notification-slide-in-2024-03-20_11.48.51.mp4

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.43%. Comparing base (23c83fe) to head (e6873ea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6272   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         267      267           
  Lines       10177    10177           
  Branches     2418     2418           
=======================================
  Hits        10119    10119           
  Misses         58       58           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya acelaya requested a review from robertknight March 20, 2024 13:27
src/sidebar/components/NotebookView.tsx Outdated Show resolved Hide resolved
<Button
onClick={applyPendingUpdates}
unstyled
classes={classnames(
'flex gap-1.5 items-center py-1 px-2',
'rounded shadow-lg bg-gray-900 text-white',
'rounded shadow-lg bg-gray-900 text-white whitespace-nowrap',
Copy link
Member

Choose a reason for hiding this comment

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

What happens without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this

image

I suppose it is because the container is now position: absolute. Perhaps we could try setting the container with w-full. I can try that.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is because the container is now position: absolute.

You need to figure out how the sizing of the element is determined when whitespace-nowrap is missing.

@@ -23,6 +23,7 @@ export default {
'fade-out': 'fade-out 0.3s forwards',
'pulse-fade-out': 'pulse-fade-out 5s ease-in-out forwards',
'slide-in-from-right': 'slide-in-from-right 0.3s forwards ease-in-out',
'updates-notification-slide-in': 'updates-notification-slide-in 0.3s ease-in'
Copy link
Member

Choose a reason for hiding this comment

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

This name is perhaps overly specific. How and why does this animation differ from slide-in-from-right which sounds very similar for example? It looks like the main difference is that it doesn't start fully off-screen and slide all the way across but rather starts only a few pixels away from its final destination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The slide-in-from-right animation animates the left property, so that it comes all the way from outside the viewport, until it hits the left side.

This new animation animates the right property, so that it comes 15px out the right side of the viewport and until it gets fully aligned to the right.

I thought about the too-specific name, but then I realized animations are implicitly coupled with other styles on the component they are used on. That's why for example slide-in-from-right could not be used here.

I considered addressing that by just using a more "detailed" animation name, but then I saw we have other examples in which the animation name references the actual component it is used with, like adder-pop-down and adder-pop-up.

All approaches have pros and cons, and I don't really have a strong opinion, so I thought it was better to start as specific as possible, and if we eventually see this animation can be useful somewhere else, then we can consider naming it more generically.

@acelaya acelaya force-pushed the updates-notif-animation branch from d44315d to 1d8ab82 Compare March 21, 2024 09:32
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of notes.

src/sidebar/components/NotebookView.tsx Outdated Show resolved Hide resolved
<Button
onClick={applyPendingUpdates}
unstyled
classes={classnames(
'flex gap-1.5 items-center py-1 px-2',
'rounded shadow-lg bg-gray-900 text-white',
'rounded shadow-lg bg-gray-900 text-white whitespace-nowrap',
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is because the container is now position: absolute.

You need to figure out how the sizing of the element is determined when whitespace-nowrap is missing.

@acelaya acelaya force-pushed the updates-notif-animation branch from 1d8ab82 to e6873ea Compare March 21, 2024 13:23
@acelaya acelaya merged commit 6b85b3d into main Mar 21, 2024
4 checks passed
@acelaya acelaya deleted the updates-notif-animation branch March 21, 2024 13:25
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.

2 participants