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.

Sign up for GitHub

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

Jump to bottom

Detailed deletion prompt #30102

Closed
wants to merge 2 commits into from
Closed

Detailed deletion prompt #30102

wants to merge 2 commits into from
+1 −1

Conversation

Remmbrandt08
Copy link

@Remmbrandt08 Remmbrandt08 commented Oct 3, 2024 •
edited by Joehuu
Loading

Specifies exactly what the action being performed does. Modifies BeatmapDeleteDialog.cs to do this.

Attempts to resolve #30071

@Remmbrandt08 Remmbrandt08 marked this pull request as draft October 3, 2024 14:02
@peppy peppy self-requested a review October 4, 2024 06:29
stanriders
stanriders reviewed Oct 4, 2024
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@Remmbrandt08
detailed deletion prompt
d09795a 
remove stray modification to gitignore

changed dialog text

detailed deletion prompt

remove stray modification to gitignore

changed dialog text
@Remmbrandt08 Remmbrandt08 marked this pull request as ready for review October 4, 2024 12:07
smoogipoo
smoogipoo requested changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@smoogipoo smoogipoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It sounds very aggressive...

  • "Are you sure you want to delete <>?"
  • "Delete <>?"

Would like second opinion on the appropriate messaging. The dialog with the current header-body style doesn't lend too much freedom here...

There is precedent for the above at least used in the tournament client:

Maybe copy the messaging there?

osu.Game/Screens/Select/BeatmapDeleteDialog.cs
BodyText = $@"{beatmapSet.Metadata.Artist} - {beatmapSet.Metadata.Title}";
BodyText = $@"You are DELETING: {beatmapSet.Metadata.Artist} - {beatmapSet.Metadata.Title}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs localisation added.

@smoogipoo
Copy link
Contributor

Also, it would be good to create a common class for this:

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

Also, it would be good to create a common class for this:

These all inherit DangerousActionDialog though...? What would you be looking for here?

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 7, 2024 •
edited
Loading

My initial idea was something like a DeleteActionDialog : DangerousActionDialog if a general "Delete <>?" was to be used, but it'd be fine to remain as is if there were target-specific localisation strings instead:

  • "Delete beatmap <>?"
  • "Delete score <>?"

etc... as the tournament client strings are.

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

Time proves to be a flat circle yet again.

@peppy
Copy link
Sponsor Member

peppy commented Oct 7, 2024

Closing this PR

@peppy peppy closed this Oct 7, 2024
@peppy peppy mentioned this pull request Oct 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stanriders stanriders stanriders left review comments

@smoogipoo smoogipoo smoogipoo requested changes

@peppy peppy Awaiting requested review from peppy

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deletion dialogs all have broken messaging
5 participants
@Remmbrandt08 @smoogipoo @bdach @peppy @stanriders