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 player settings to multi spectator screen #30749

Merged
merged 11 commits into from
Dec 2, 2024

Conversation

Sheppsu
Copy link
Contributor

@Sheppsu Sheppsu commented Nov 18, 2024

I'm creating this for #29709 so the analysis settings have a place to go, but it's also nice to have the player settings on the multi spectator screen.

Looks like this:

2024-11-18.03-56-06.mp4

@peppy
Copy link
Member

peppy commented Nov 18, 2024

If we're going to do this, please make it do the settings expand when moving the cursor near the right of the screen, or when hovering the cog.

@Sheppsu
Copy link
Contributor Author

Sheppsu commented Nov 23, 2024

Now expands and contracts from hovering the right side of the screen:

2024-11-23.01-06-42.mp4

@peppy
Copy link
Member

peppy commented Nov 27, 2024

@Sheppsu sorry to kind of take over and rewrite this to some extent, but I had some major issues with the structure, especially when trying to make use of this in more places than you originally envisaged.

See what you think of my changes, I've committed them.

@Sheppsu
Copy link
Contributor Author

Sheppsu commented Nov 27, 2024

Makes sense. Changes look good 👍

peppy
peppy previously approved these changes Nov 28, 2024
@peppy
Copy link
Member

peppy commented Nov 28, 2024

Requesting a second opinion as I wrote a lot of this now.

@peppy peppy requested a review from a team November 28, 2024 05:58
@bdach bdach self-requested a review November 28, 2024 12:32
@bdach
Copy link
Collaborator

bdach commented Nov 28, 2024

Are the settings supposed to show during normal gameplay...?

2024-11-28.13-37-58.mp4

I feel like that's not going to go over well with users...

@peppy
Copy link
Member

peppy commented Nov 29, 2024

Nope, that looks like a regression from my refactors.

@peppy peppy self-requested a review December 1, 2024 09:23
@pull-request-size pull-request-size bot removed the size/M label Dec 1, 2024
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

seems fine

@bdach bdach merged commit 52b8753 into ppy:master Dec 2, 2024
7 of 10 checks passed
@Sheppsu Sheppsu deleted the multi-spectator-settings-sidebar branch December 11, 2024 02:51
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