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

Refresh dropdown appearance & add themed variant to settings sidebar #15070

Merged
merged 16 commits into from Oct 14, 2021

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Oct 12, 2021

PRing primarily for comment as I'm not sure if the direction will be found agreeable.

Based (somewhat loosely at this point) on the following designs: https://www.figma.com/file/IjrU0u7MFdgmxDofqTlsZb/UI%2FClient-Settings?node-id=0%3A1

2021-10-12.23-56-17.mp4

I say that the updated appearance is based "somewhat loosely" as most of the appearance is unspecified on the design (no hover state, no expanded state, no scrollbar spec). I filled those gaps with what I thought to be my best judgement; the goal was to keep the spirit of the old design but use the new colour palette.

All spacing/layout changes here I consider to be objective improvements and as such they have been applied globally. The more controversial part of this (in my view) is going to be the addition of ThemedDropdown for the colouring. The reason why it is a thing is that I tried the simplest thing first - shoving the OverlayColourProvider into the base controls - but there are places that cache overlay colours and have somewhat transitional colour schemes, and that ended up looking terrible there:

editor setup screen: awful contrast with bg playlists room creation: awful contrast with bg
image image

Because those cases were mostly transitional as-is anyway, the idea was to have a separate class that opts into the overlay colour scheme, and gradually replace the existing non-themed usages as designs are being updated.

Unfortunately, though, in the end I had to make the OverlayColourProvider dependency nullable anyway, because SettingsItem (which is the only consumer of the themed dropdown) is shared between the settings sidebar, mod settings control, and the tournament client. So at that point I would have either needed to do what I did or needed to add a ThemedSettingsItem or whatever. And both options seem just about as bad to me, except the first one is shorter/easier.

@peppy
Copy link
Sponsor Member

peppy commented Oct 13, 2021

While I'm not against this structure, I'm leaning towards just making the bad contrast usages look bad for now or fix them in this PR, rather than make new classes for this. It feels like otherwise we're just going to get further away from the goal of moving everything to colour providers, unless that is seen as an immediate follow-up PR to this.

Rationale is that the first thing that stood out here is the menu items having a pitch black caret, which should not be the case. Which means the new class is going to also require an overridden CreateMenuItem method and things are going to get a bit confusing.

@bdach
Copy link
Collaborator Author

bdach commented Oct 13, 2021

Sure. I'll look into applying the colours globally and applying local fixes in places where that looks bad, then.

@peppy
Copy link
Sponsor Member

peppy commented Oct 13, 2021

I'll leave the chevron update to you then. Should be anything from a colour provider that isn't black

Colour = Color4.Black,

@bdach
Copy link
Collaborator Author

bdach commented Oct 13, 2021

I've applied the recolouring changes directly to OsuDropdown as proposed, and adjusted a few usage sites to match. Screenshots of relevant modified places follow.

editor setup screen multiplayer/playlists lounge multiplayer/playlists room creation subscreen
osu_2021-10-13_23-04-21 osu_2021-10-13_23-04-32 osu_2021-10-13_23-04-58

Colouring changes applied there are educated guesses based on the latest WIP designs for editor and multiplayer.

smoogipoo
smoogipoo previously approved these changes Oct 14, 2021
@peppy
Copy link
Sponsor Member

peppy commented Oct 14, 2021

I've made some changes to the animation of the chevron and background fill just to freshen things up.

In the interest of making forward progress, I think this is fine to merge. That said, a next step would be completely ripping out the AccentColour flow that's currently being used. Main reasoning is that I'd want the item selected and hovered background colours to be a different shade, but with the current single-accent-colour logic that's not easily possible.

peppy
peppy previously approved these changes Oct 14, 2021
@peppy peppy enabled auto-merge October 14, 2021 04:42
@peppy peppy disabled auto-merge October 14, 2021 05:40
@peppy peppy merged commit 464bcd2 into ppy:master Oct 14, 2021
@bdach bdach deleted the dropdown-refresh branch October 14, 2021 05:55
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