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

fix(NcAppSidebar): make closing animation less glitchy #5608

Merged
merged 2 commits into from
May 20, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 17, 2024

☑️ Resolves

  • Fixes a number of issues with animations:
    1. Sidebar doesn't slide right, it becomes "narrow", actually moving content inside (especially with multiline title)
    2. There is a moment when there is close and open at the same position, because the close button is not moving
    3. It was not width but max-width animated. It means that if max-width: 500px and the current width is 27vw = 400px - there is no animation for the first 100px making a delay
    4. New --app-sidebar-offset changes too immediate, making a jump effect

🖼️ Screenshots

🏚️ Before 🏡 After
before-quick after-quick
before-slow after-slow
mobile-before-quick mobile-after-quick
mobile-before-slow mobile-after-slow

🚧 Tasks

  • Use transition on --app-sidebar-offset
  • Position Close button relative to the sidebar
  • Slide with margin instead of max-width
  • Test on mobile

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress feature: app-sidebar Related to the app-sidebar component labels May 17, 2024
@ShGKme ShGKme added this to the 8.12.0 milestone May 17, 2024
@ShGKme ShGKme self-assigned this May 17, 2024
@ShGKme ShGKme force-pushed the fix/nc-app-sidebar--fix-animations branch 2 times, most recently from 594d829 to 32c0bc2 Compare May 17, 2024 13:40
@Antreesy
Copy link
Contributor

Antreesy commented May 17, 2024

Wouldn't transform: translate be more appropriate here to use for animation?

@ShGKme
Copy link
Contributor Author

ShGKme commented May 17, 2024

Wouldn't transform: translate be more appropriate here to use for animation?

No, it would keep a large empty place when the sidebar is closing and then immediately resize page content.

An alternative could be to use absolute position together with translate, so sidebar would open on top of page content without an empty space. However, it would also make content resize immediately.

So I used the same approach as we are using now on NcAppNavigation. We could go with an alternative solution if there are performance issues.

@ShGKme
Copy link
Contributor Author

ShGKme commented May 17, 2024

@Antreesy see also gifs here: #5389 (review)

@@ -1116,11 +1116,21 @@ export default {
</script>

<style lang="scss">
// Allows to use transition over a custom CSS property (CSS Variable)
// Ignored on old browsers resulting in slightly noticeable jump
@property --app-sidebar-offset {
Copy link
Contributor

Choose a reason for hiding this comment

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

fancy!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't work in Firefox, except nightly build. But when it doesn't work, it's just a little more jumping

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Looks smooth when testing

@ShGKme ShGKme marked this pull request as ready for review May 17, 2024 20:08
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/nc-app-sidebar--fix-animations branch from 32c0bc2 to 6291ce1 Compare May 17, 2024 20:15
@ShGKme
Copy link
Contributor Author

ShGKme commented May 17, 2024

Rebased onto master, re-checked, no changes, ready to review

@ShGKme
Copy link
Contributor Author

ShGKme commented May 17, 2024

Snapshots should be updated. Previously they had no close button because it has absolute position without relative on .app-sidebar. It worked on the web-page, but not on cypress render.

I'll upload new snapshots when I have a stable internet connection.

@susnux susnux modified the milestones: 8.12.0, 8.12.1 May 19, 2024
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@marcoambrosini marcoambrosini merged commit 532a257 into master May 20, 2024
19 checks passed
@marcoambrosini marcoambrosini deleted the fix/nc-app-sidebar--fix-animations branch May 20, 2024 07:57
@ShGKme
Copy link
Contributor Author

ShGKme commented May 20, 2024

/backport to next

@marcoambrosini
Copy link
Contributor

It worked for me in files, but now testing in talk it seems that the sidebar does not close anymore @ShGKme

Screen.Recording.2024-05-20.at.12.27.39.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug Something isn't working feature: app-sidebar Related to the app-sidebar component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants