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(MediaSettings): jumping on devices/backgrounds switch #12815

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jul 25, 2024

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
before-media final

🚧 Tasks

  • Add a new simple tabs component (not very reusable atm)
    • With panel switch transition with permanent height independent of the current tab's content height
    • Also, improve tabs a11y, except arrows tabs switch
  • Add a TransitionExpandDown component
    • Allows having a slide down transition but keep taking an actual height
    • Not a part of TransitionWrapper, because it is a bit more complex than adding name and CSS

Checked:

  • Initially opening any tab, not only the first one. Same with closing.
  • Closing tabs during opening and vice versa

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 25, 2024

@jancborchardt

We can easily remove the jumping when switching between tabs. But there is still a jump on opening a tab for the first time.

What do you think?

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

We can easily remove the jumping when switching between tabs.

Nice, looks great! :)

But there is still a jump on opening a tab for the first time.

Can we have a quick vertical size increase / decrease animation on opening the tab initially (and closing it as well)? We can also use either --animation-slow or --animation-quick for that, whatever looks nicer.

@jancborchardt
Copy link
Member

And btw, the first time you switch to the "Backgrounds" tab, there is no left/right animation. Is that an artifact in the screen recording or a bug?

@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 25, 2024

Can we have a quick vertical size increase / decrease animation on opening the tab initially (and closing it as well)?

Sure

And btw, the first time you switch to the "Backgrounds" tab, there is no left/right animation. Is that an artifact in the screen recording or a bug?

If the switch happens too fast after opening the first tab, there is a small glitch — a browser needs some time to load and render quite huge images...

You can see it on the before example. Images are shown after some time. The same happens for after, but hidden.

Will have a look if we can prevent it.

Also, on gif it's a bit more noticeable than in reality

@ShGKme ShGKme self-assigned this Jul 27, 2024
@ShGKme ShGKme added this to the 💙 Next Beta (30) milestone Jul 27, 2024
@ShGKme ShGKme force-pushed the fix/media-settings-ui branch 2 times, most recently from 0f963f9 to efdd142 Compare July 27, 2024 21:11
@ShGKme ShGKme marked this pull request as ready for review July 29, 2024 08:20
@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 29, 2024

@jancborchardt Updated gif. What do you think?

@ShGKme ShGKme requested a review from Antreesy July 29, 2024 08:21
@ShGKme ShGKme force-pushed the fix/media-settings-ui branch 2 times, most recently from 7c888d2 to fec1555 Compare July 29, 2024 08:24
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Looks good!

@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 29, 2024

Rebased and squashed

@ShGKme ShGKme enabled auto-merge July 29, 2024 10:29
@ShGKme ShGKme merged commit d5f25d8 into main Jul 29, 2024
46 checks passed
@ShGKme ShGKme deleted the fix/media-settings-ui branch July 29, 2024 10:32
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@ShGKme this looks so damns smooth now, awesome work! 😲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants