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): animation glitch on toggle #5389

Merged
merged 1 commit into from Mar 20, 2024

Conversation

RayBB
Copy link
Contributor

@RayBB RayBB commented Mar 11, 2024

Signed-off-by: RayBB RayBB@users.noreply.github.com

☑️ Resolves

Apologies, I cannot test this outside of the browser devtools because of #5339 but it is a simple enough change I hope someone else can help test/verify until I can get a local instance working. Thanks!

Edit: Finally I was able to test in a local instance and it worked great, looks the same as the video I put below.

🖼️ Screenshots

The "after" video shows before/after

🏚️ Before 🏡 After
image https://github.com/nextcloud/server/assets/921217/0303ae9c-a791-4ed1-98c1-0755f81aa259

🚧 Tasks

  • ...

🏁 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 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels Mar 12, 2024
@ShGKme ShGKme added this to the 8.10.1 milestone Mar 12, 2024
@ShGKme ShGKme changed the title fix sidebar animation glitch fix(NcAppSidebar): animation glitch on toggle Mar 12, 2024
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Very cool, thanks!

Slowed down to better see the difference:

Before After
Before after

This could be a little bit slower than before in performance, because it causes animation is width change of the main content, triggering resize observers. But should be fine, NcAppSidebar does similar change.

If this accurse bad for performance, we can make it absolute during animation:

Alternative

@ShGKme ShGKme requested review from susnux and skjnldsv March 12, 2024 22:10
@ShGKme
Copy link
Contributor

ShGKme commented Mar 12, 2024

/backport to next

@skjnldsv
Copy link
Contributor

Please also check this works on very narrow screens like mobile at 320/400px

@ShGKme ShGKme modified the milestones: 8.10.1, 8.11.0, 8.11.1 Mar 15, 2024
@RayBB RayBB requested a review from ShGKme March 20, 2024 00:10
@RayBB
Copy link
Contributor Author

RayBB commented Mar 20, 2024

@ShGKme I updated to fix this for the mobile case.

What master looks like now on small screens
small.mp4
And here is what it looks like desktop/mobile with my current changes.
Files.-.Nextcloud.-.20.March.2024.1.mp4

Please let me know if I should change anything else. Cleaning up commits now.

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Great

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.62%. Comparing base (6f1a4c9) to head (957f0ac).
Report is 289 commits behind head on master.

❗ Current head 957f0ac differs from pull request most recent head 85f19d5. Consider uploading reports for the commit 85f19d5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5389      +/-   ##
==========================================
- Coverage   39.34%   38.62%   -0.72%     
==========================================
  Files         139      142       +3     
  Lines        3688     4958    +1270     
  Branches      810     1497     +687     
==========================================
+ Hits         1451     1915     +464     
- Misses       2021     2954     +933     
+ Partials      216       89     -127     

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

Signed-off-by: RayBB <RayBB@users.noreply.github.com>
@ShGKme
Copy link
Contributor

ShGKme commented Mar 20, 2024

I've rebased the branch onto master to check if dependency updates help with Cypress error on CI.

@susnux susnux merged commit a228c45 into nextcloud-libraries:master Mar 20, 2024
12 of 14 checks passed
@susnux
Copy link
Contributor

susnux commented Mar 20, 2024

/backport to next

Copy link

backportbot bot commented Mar 20, 2024

The backport to next failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout next
git pull origin next

# Create the new backport branch
git checkout -b backport/5389/next

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 85f19d51

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/5389/next

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud-libraries:backport/5389/next."}


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews 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.

[Bug]: Navigation Menu (sidebar) CSS transition is janky with sharp corners
4 participants