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

Implement beatmap options popover #24712

Merged
merged 14 commits into from Sep 6, 2023
Merged

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Sep 4, 2023

osu Game Tests_K92q8e3VK4

Some differences to the proposed design:

  • no arrow (to make things simple/consistent as no other popover has it yet)
  • there were inconsistent measurements so I did my best to find the average/best numbers
  • button sorting hasn't changed because of keys being assigned by index (muscle memory)
  • icons were unchanged from old design (i.e. trash) as the design claims that it is using the same icons the game currently uses (see top on figma)

I had to take a different approach to add the buttons to the popover content since popovers are reconstructed every time.

The old BeatmapOptionsOverlay had SongSelect adding the buttons, and if you wanted to make an isolated test scene, you would have to copy the adding logic. It is now local by using DI.

Here's a WIP branch for the footer integrated into the game: master...Joehuu:osu:footer-redesign

@ItsShamed
Copy link
Sponsor Contributor

I saw in the GIF that it says "rewind" when you hold shift (for when you're rewinding random). I think this should also change the icon to better signify that. (it isn't in the Figma, but it would be a nice addition)

@bdach bdach requested a review from peppy September 4, 2023 09:55
/// <summary>
/// "General"
/// </summary>
public static LocalisableString General => new TranslatableString(getKey(@"general"), @"General");
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Not sure about having "general" in so many places for translation.

public static LocalisableString GeneralHeader => new TranslatableString(getKey(@"general_header"), @"General");

is at least one other example.

Maybe these should be in the general localisations as a single string?

@peppy peppy enabled auto-merge September 6, 2023 06:16
@bdach bdach disabled auto-merge September 6, 2023 07:30
@bdach bdach merged commit 65e5169 into ppy:master Sep 6, 2023
15 of 17 checks passed
@Joehuu Joehuu deleted the beatmap-options-popover branch September 6, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BeatmapOptionsOverlay is missing new design
4 participants