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 basic animation to new footer and buttons #28184

Merged
merged 5 commits into from
Jun 8, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented May 16, 2024

Preview:

CleanShot.2024-05-16.at.07.44.16.mp4

There are no animations defined anywhere in the reference designs so I rolled up my own. The intention of this PR is to at least have something there that can be tweaked.

@peppy peppy self-requested a review May 28, 2024 11:31
Copy link
Member

@peppy peppy 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 visually, some code concerns.

Size = new Vector2(BUTTON_WIDTH, BUTTON_HEIGHT);

Child = new Container
Child = TopLevelContent = new Container
Copy link
Member

Choose a reason for hiding this comment

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

I'm not huge on exposing this, but can live with it.

That said, having the animations applied by ScreenFooter directly on this is a no-go for me. I think that needs to be moved internally to the button class itself.

Also needs some commenting on why the animations are being applied to this container, not the button itself. I'm sure there's valid reasoning but it needs to be explicitly stated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the animations inside of the button class as mentioned, and I've replaced TopLevelContent with just Content (which points to a local container since the button class inherits from ClickableContainer).

I'm not entirely sure if commenting is necessary though, the button class doesn't apply transitions on this anywhere else, and there's a precedent for this in ShearedButton, where Content is used to rescale the button instead of this, so that if there's a button next to this one all inside of a flow container, it won't move from its place.

…m base implementation instead

The point is to apply the transitions against a container that's inside of `ScreenFooterButton`, because the `ScreenFooterButton` drawable's position is being controlled by the flow container it's contained within, and we cannot apply the transitions on it directly.
@peppy peppy merged commit 7fc6ad5 into ppy:master Jun 8, 2024
10 of 17 checks passed
@frenzibyte frenzibyte deleted the footer-v2-transitions branch June 8, 2024 11:10
@frenzibyte frenzibyte self-assigned this Jul 10, 2024
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.

2 participants