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

Improve animation of new song select footer buttons #28142

Merged
merged 4 commits into from May 12, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented May 10, 2024

Changed to match animations of ShearedButton. Not a good idea to inherit from it since the footer button designs differ from ShearedButton slightly + we don't want the scale-in effect in footer buttons.

I have also fixed the "options" button not actually hiding when pressing on it while in open state. I'm not entirely sure about my approach but, well...it works™.

Preview:

CleanShot.2024-05-10.at.08.26.26.mp4

@Joehuu
Copy link
Member

Joehuu commented May 10, 2024

I have also fixed the "options" button not actually hiding when pressing on it while in open state.

When I was implementing that, I mostly was following other popovers triggered by a button (e.g. add preset button). Pressing the button while the popover is open triggers the DismissOnMouseDownContainer on PopoverContainer first then the button action which opens the popover again. I've found that behavior weird and expected toggling like done locally with options button.

@frenzibyte
Copy link
Member Author

Well, yeah, that's what I'm achieving in this PR, having it not open the popover again when clicking on the button while it's already in an open state.

@peppy
Copy link
Sponsor Member

peppy commented May 10, 2024

I dunno, I didn't mind it reopening each click personally.

@frenzibyte
Copy link
Member Author

frenzibyte commented May 10, 2024

Hmm, to me it's an established behaviour that clicking on anything that opens something should close it if it's already open (e.g. dropdowns, toolbar toggle buttons). In addition, keeping the behaviour as-is will already introduce an inconsistency against the "mods" button, since that also acts like a toggle (closes when clicking if open...or at least that's how it's supposed to behave).

I guess you could make the "options" button not behave like a toggle altogether, but still, it feels off to me.

@peppy
Copy link
Sponsor Member

peppy commented May 10, 2024

It's fine to change it. All the places in operating systems which had behaviour similar to how this used to behave (prior to this PR) have disappeared over the last decade so it does seem like the correct direction.

@Joehuu
Copy link
Member

Joehuu commented May 10, 2024

Well, yeah, that's what I'm achieving in this PR, having it not open the popover again when clicking on the button while it's already in an open state.

What I was trying to convey is other popovers opened with buttons should also be changed to this new behavior for consistency.

keeping the behaviour as-is will already introduce an inconsistency against the "mods" button, since that also acts like a toggle (closes when clicking if open...or at least that's how it's supposed to behave

As you said here, the same applies to the add preset button and presets:

Kapture.2024-05-10.at.10.58.48.mp4

@frenzibyte
Copy link
Member Author

frenzibyte commented May 10, 2024

I see. For other cases facing the same issue, I would open an issue and/or solve them separately, as long as we agree that the behaviour proposed by this PR is more correct.

@peppy peppy self-requested a review May 11, 2024 14:32
@pull-request-size pull-request-size bot added size/M and removed size/L labels May 12, 2024
@frenzibyte frenzibyte changed the title Improve animation of new song select footer buttons (and fix "options" button behaviour) Improve animation of new song select footer buttons May 12, 2024
@peppy peppy merged commit 147ebb6 into ppy:master May 12, 2024
15 of 17 checks passed
@frenzibyte frenzibyte deleted the footer-v2-button-animation branch May 12, 2024 03:15
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.

None yet

3 participants