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 visual component for assigning user tags to beatmaps #32221

Merged
merged 9 commits into from
Mar 12, 2025

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Mar 4, 2025

2025-03-04.13-10-41.mp4

Visual part for #31913. Opening separately for appropriate visual UI adjustments.

Also mostly ready to be hooked up to the results screen, pending merge of ppy/osu-web#11951 - but not hooked up quite yet.

Visual part for ppy#31913. Opening
separately for appropriate visual UI adjustments.

Also mostly ready to be hooked up to the results screen, pending merge
of ppy/osu-web#11951.
@peppy peppy self-requested a review March 8, 2025 11:57
@peppy
Copy link
Member

peppy commented Mar 8, 2025

Hmm, I think we're going to need a search box for tags (probably autofocused) selection, I guess that will be a follow-up effort?

@bdach
Copy link
Collaborator Author

bdach commented Mar 8, 2025

I'd honestly rather than do it here probably. It shouldn't be a huge undertaking.

Unless you're uncomfortable with the diff size here I suppose.

Comment on lines +377 to +378
voteCount.Value -= 1;
voted.Value = false;
Copy link
Member

Choose a reason for hiding this comment

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

I guess the only weird thing I've seen is that a tag returning to 0 count doesn't disappear and instead stays visible with a 0. Maybe fine, not sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied in 3d4dd85

@peppy
Copy link
Member

peppy commented Mar 9, 2025

I'd honestly rather than do it here probably. It shouldn't be a huge undertaking.

Sounds fine. Diff so far is palatable so feel free to go ahead.

@bdach
Copy link
Collaborator Author

bdach commented Mar 10, 2025

Search box added in 00127b3

@peppy peppy self-requested a review March 11, 2025 09:36
@peppy
Copy link
Member

peppy commented Mar 11, 2025

I've made some changes. Not hugely keen on the number of subclasses, but adjusting the naming helped things become acceptable to my eye.

@bdach
Copy link
Collaborator Author

bdach commented Mar 11, 2025

Not hugely keen on the number of subclasses

That's mostly because I see a lot of these as single-use but point taken 👍 No objections to the changes.

@peppy peppy merged commit ad04b5b into ppy:master Mar 12, 2025
8 of 10 checks passed
@bdach bdach deleted the user-tags-visual branch March 12, 2025 06:57
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.

3 participants