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 platform issue tags #11428

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

venix12
Copy link
Member

@venix12 venix12 commented Aug 11, 2024

This adds buttons for "[osu!lazer]", "[osu!stable]", and "[osu!web]" thread title tagging, as of request of help forum moderators.

Uses separate const from regular tags as it doesn't have icon thing, and is supposed to be visible in thread names in the listing.

Also, genuinely had no idea for icons so they could be better possibly, though a mod-only feature so probably shouldn't matter as much 🤷. It is text now.

In order: osu!lazer, osu!stable, osu!web
image

@peppy
Copy link
Member

peppy commented Aug 12, 2024

Do these buttons actually fit? Can you provide a screenshot showing how this looks?

Also I don't think we need localisation for any of this.

@venix12
Copy link
Member Author

venix12 commented Aug 12, 2024

Updated the OP with a screenshot.

As for translations, it's built into issue tags workflow so is easier to just go that way rather than omitting it, it's only tooltips and success messages that get translated anyway, the tag itself doesn't.

@peppy
Copy link
Member

peppy commented Aug 12, 2024

is it possible to replace the icons with text? might make it more legible than almost-understandable icons. laz / stb / web maybe.

@venix12
Copy link
Member Author

venix12 commented Aug 12, 2024

Guess that works, done so.

function platform_issue_text($issue)
{
return match ($issue) {
'lazer' => 'laz',
Copy link
Member

Choose a reason for hiding this comment

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

lzr feels like a better acronym than laz, thoughts?

Suggested change
'lazer' => 'laz',
'lazer' => 'lzr',

@@ -10,7 +10,7 @@
@include('forum.topics._moderate_move', compact('topic'))

@if ($topic->isIssue())
@foreach ($topic::ISSUE_TAGS as $type)
@foreach (array_merge($topic::ISSUE_TAGS, $topic::PLATFORM_ISSUE_TAGS) as $type)
@include("forum.topics._issue_tag_{$type}")
Copy link
Member

Choose a reason for hiding this comment

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

or

@include('forum.topics._issue_tag', ['issueTag' => $type])

and no extra blade templates

Comment on lines +820 to +822
if (in_array($tag, static::PLATFORM_ISSUE_TAGS, true)) {
$tag = "osu!{$tag}";
}
Copy link
Member

Choose a reason for hiding this comment

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

probably move this logic to its own method cuz it comes up 3 times (tagForTopicTitle() or something)

alternatively include the prefix in each PLATFORM_ISSUE_TAGS

@cl8n
Copy link
Member

cl8n commented Aug 13, 2024

also I think it would be good to combine the issue_tag_{tag} translations into just one set that takes the tag name as input, so that translators don't have to write a bunch of almost-duplicate translations either. but that's more like refactoring what was already here i guess

@nanaya
Copy link
Collaborator

nanaya commented Aug 14, 2024

feels like the tags should just be moved to a menu with this amount

@peppy
Copy link
Member

peppy commented Aug 14, 2024

I wouldn't be against that.

@Blooshing
Copy link
Contributor

Making a new panel for the buttons would be nice, I wouldn't mind that. Though this design works fine as well since only a few people are using the buttons it really wouldn't matter either way.

Thanks for the quick work Venix, I appreciate it.

@@ -103,6 +103,12 @@ class Topic extends Model implements AfterCommit
'topic_title' => 100,
];

const PLATFORM_ISSUE_TAGS = [
'lazer',
Copy link
Collaborator

Choose a reason for hiding this comment

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

str_slug the type (class name, translation, filename) and use the actual tag string everywhere else instead? The icon helper can just generate the full content (stb etc string for the new tags and <i ...> for everything else)

@@ -25,6 +25,10 @@ class="
data-method="post"
>
<span class="btn-circle__content">
<i class="{{ issue_icon($issueTag) }}"></i>
@if (in_array($issueTag, $topic::ISSUE_TAGS, true))
Copy link
Collaborator

Choose a reason for hiding this comment

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

view forum.forums._topic also needs adjustment

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.

6 participants