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

Mod customization header/panel changes #29626

Merged
merged 10 commits into from
Sep 4, 2024
Merged

Mod customization header/panel changes #29626

merged 10 commits into from
Sep 4, 2024

Conversation

Fabep
Copy link
Contributor

@Fabep Fabep commented Aug 28, 2024

Removed on click event for the mod customization header, As of #29618 .

Renamed ModCustomisationPanelState, since only mods set the "old" Expanded panel state.

Further suggestions that could be applied to this area:

  • Refactor mods that use the ExpandedByMod state to set parameters directly, for example copy attributes from current map when you enable difficulty adjustments.
  • Keep colourprovider.light4 on the header when the mod customization panel is expanded. (I attempted to add this, but I am too stupid to understand how to remove it when the hover is lost & the panel is collapsed). 😃

@peppy
Copy link
Member

peppy commented Aug 30, 2024

@Fabep any reason this is marked as a draft?

@peppy peppy self-requested a review August 30, 2024 09:32
@bdach
Copy link
Collaborator

bdach commented Aug 30, 2024

I have big suspicions this just straight up breaks mobile btw. Haven't confirmed yet.

@Fabep
Copy link
Contributor Author

Fabep commented Aug 30, 2024

@peppy I have no clue 😎
But as bdach said something could break completely for mobile/touch device since the only change is the removal of the click event.

I probably should have attempted at adding some sort of expansion of the panel with the touchdown / touchend? event, but it didn't cross my mind until I read bdach's comment just now.

@peppy
Copy link
Member

peppy commented Aug 30, 2024

Reading it, I can't see how it would break mobile. But either way let's just undraft it for proper review.

@peppy peppy marked this pull request as ready for review August 30, 2024 09:54
peppy
peppy previously approved these changes Aug 30, 2024
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.

Haven't tested mobile but seems fine otherwise?

@peppy peppy requested a review from bdach August 30, 2024 09:56
@bdach
Copy link
Collaborator

bdach commented Aug 30, 2024

Touch was likely working only by way of the touch-to-synthesised-click pathway, which is evidenced further by stuff like this:

protected override bool OnTouchDown(TouchDownEvent e)
{
if (Enabled.Value)
{
touchedThisFrame = true;

if (!touchedThisFrame && panel.ExpandedState.Value == ModCustomisationPanelState.Collapsed)
panel.ExpandedState.Value = ModCustomisationPanelState.Expanded;

So if my hunch is correct then the panel just won't open on mobile at all.

As expected the previous touch handling would prevent the header from
working entirely when click handling was removed from the header.
@bdach
Copy link
Collaborator

bdach commented Sep 2, 2024

As I suspected, this just broke touch completely. 5211e60 should bring the behaviour more or less to parity with desktop:

Record_2024-09-02-11-37-04_25a1a32208bbcdc1d450b7aa854bb161.mp4

tl;dr: you have to touch the dropdown to open it if it's not forced open by a mod, and touch away from it to close it. Touching the dropdown header when open does nothing which matches desktop.

@peppy requesting re-check to make sure this is the UX you are looking for.

bdach
bdach previously approved these changes Sep 2, 2024
@Fabep
Copy link
Contributor Author

Fabep commented Sep 2, 2024

Keep colourprovider.light4 on the header when the mod customization panel is expanded. (I attempted to add this, but I am too stupid to understand how to remove it when the hover is lost & the panel is collapsed). 😃

Personally for UI purposes, I believe this to be the only thing that could be improved upon. The hover color is now redundant due to the fact that the only way to open the mod customization panel is through the hover (or force open by mod). It would be more clear and "cleaner" to bind the color to it being active or not, e.g dark when collapsed and light when expanded.

I've tried implementing it with the following code snippet inside of the ModCustomisationHeader class.

protected override void LoadComplete()
{
    ...
    panel.ExpandedState.BindValueChanged(v =>
    {
        IdleColour = v.NewValue switch
        {
            ModCustomisationPanelState.Expanded or
            ModCustomisationPanelState.ExpandedByMod => colourProvider.Light4,
            _ => colourProvider.Dark3,
        };
    }, true);
    ...
}

However when I try it functionally it works like the following:

2024-09-02.13-33-22.mp4

This could probably be it's own PR if it's not an easy implementation or just not implemented at all it's just a preference after all 😃

…moved it here into the commit instead ;"

This reverts commit 582ffcf.

Revert "Mod customisation header's color is now based on the state of the panel rather than the hover of the container."

This reverts commit e3457d8.
@peppy
Copy link
Member

peppy commented Sep 4, 2024

I'm reverting the colour changes. Please make them in a separate PR or something. They look weird and your code formatting is wrong.

@peppy peppy merged commit 9abaa87 into ppy:master Sep 4, 2024
9 checks passed
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.

4 participants