Skip to content

Conversation

@mou-haz
Copy link
Contributor

@mou-haz mou-haz commented Mar 17, 2025

No description provided.

@mou-haz
Copy link
Contributor Author

mou-haz commented Mar 17, 2025

SettingsExpander Expand/Collapse animations

@mou-haz mou-haz force-pushed the settings_control_expand_collapse_animations branch from 7f11b24 to 49b70df Compare March 26, 2025 19:00
@mou-haz
Copy link
Contributor Author

mou-haz commented Mar 26, 2025

@NotYoojun can u take a look at this, please?

@NotYoojun NotYoojun self-requested a review March 27, 2025 05:44
Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

Thanks for your good work! However, there are a few problems that need to be resolved before merging. The most important one is how the animation impacts the layout.

Outer Layout

As you can see, when expanding a CommunityToolkit expander, the component will take the space immediately, and in this PR, the space grows along the animation.

Current behavior in this PR:

Animation

Animation

Expected behavior in CommunityToolkit:

Animation

Animation

Inner Layout

What's more, the content in the Expander doesn't seems to be arranged correctly.

Current behavior:

Animation

Expected behavior:

Animation

I have to apologize that the GIFs may not be clear and easy enough to understand. You can download the CommunityToolkit Gallery from Microsoft Store, and you'll feel the actual differences.

Incorrect Duration

Additionally, the duration of this animation seems to be incorrect.

Please make sure that everything is the exact same as native Windows UI or CommunityToolkit. We're be so appreciated if you could fix these problems above. Thanks!

@mou-haz mou-haz marked this pull request as draft March 27, 2025 13:36
@mou-haz mou-haz marked this pull request as ready for review March 28, 2025 02:45
@mou-haz mou-haz requested a review from NotYoojun March 28, 2025 02:46
Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

Hi, thanks for taking time into this. There's only one last problem before merging!

I guess this is very easy to fix:

image

The content area should be aligned to the header.
After this one, I guess this PR can be merged. Thanks a lot for your hard work!

@mou-haz mou-haz marked this pull request as draft March 28, 2025 13:02
@mou-haz mou-haz requested a review from NotYoojun March 28, 2025 13:32
@mou-haz mou-haz marked this pull request as ready for review March 28, 2025 13:32
@NotYoojun
Copy link
Member

Well done! We really appreciate your dedication to open source. Your contribution is valuable to us and the community. Keep up the great work!

@NotYoojun NotYoojun merged commit aa51c80 into iNKORE-NET:main Mar 29, 2025
@NotYoojun
Copy link
Member

By the way, this closes #127

@NotYoojun NotYoojun linked an issue Mar 29, 2025 that may be closed by this pull request
6 tasks
@mou-haz
Copy link
Contributor Author

mou-haz commented Mar 29, 2025

Well done! We really appreciate your dedication to open source. Your contribution is valuable to us and the community. Keep up the great work!

Thank you.
The reviews were really insightful and helpful

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.

Add expand animation for SettingsExpander

2 participants