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 country flag tooltips in wiki #10668

Merged
merged 11 commits into from
Nov 6, 2023
Merged

Conversation

Swekka
Copy link
Contributor

@Swekka Swekka commented Oct 22, 2023

Closes #10661

Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

I'm pretty sure we don't want to be doing a db query for every single flag

@Swekka
Copy link
Contributor Author

Swekka commented Oct 23, 2023

I just realized that if AU happens to be in the test db this test will always fail, since the actual html will have a title but the expected won't, but I have no idea what to do about that since the countries in the test db are always random

@notbakaneko
Copy link
Collaborator

I don't think we want to make a network request to redis for every flag either; use a local cache like https://github.com/ppy/osu-web/blob/3b91b06/app/Libraries/Groups.php or https://github.com/ppy/osu-web/blob/3b91b06/app/Libraries/Medals.php does it.

@notbakaneko
Copy link
Collaborator

I mean making a service object to cache the country names like Groups does and adding it to AppServiceProvider https://github.com/ppy/osu-web/blob/master/app/Providers/AppServiceProvider.php#L39-L43

Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

Fetch the countries (preferably just the name and country code) once, store it in an a Map or keyed array and do the lookup against the Map/array as needed.

Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

    private function allByCode(): Collection
    {
        return $this->memoize(__FUNCTION__, fn () => Country::get(['acronym', 'name'])->keyBy('acronym'));
    }

    public function byCode(string $code): ?Country
    {
        return $this->allByCode()->get($code);
    }


class Countries
{
use Memoizes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is needed for LocalCacheManager to manage resetting the local cache with resetMemoized()

@notbakaneko notbakaneko merged commit 6f5188e into ppy:master Nov 6, 2023
3 checks passed
@Swekka Swekka deleted the wiki-flag-country-name branch November 7, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Country flags in osu! wiki don't have tooltips
2 participants