-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix "Display 1 to 0 of 0 URLs" on admin list page #3910
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
Conversation
You know how it is - you got something perfect, a bleach-white shirt, and people will notice the tiniest speck. This one fixes the message "Display 1 to 0 of 0 URLs." on a new installation. Best regards from your friendly autist. ;-)
Nice catch, never noticed. This said, you hardcoded the message, making it untranslatable. See https://yourls.org/docs/development/i18n for how to make it |
OMFG you got me with my pants down (this time!) I'm very sorry for the oversight, and I hope you can find it in the bottom of your heart to forgive me. Of course it should take into account whatever i18n technique you're using. I acted rashly and was still in rage mode earlier. That said, I guess your reply means you would be rather inclined to accepting the pull if I put in some real work. :D Right now I'm on my way out, but will look into everything properly later on, cheers! |
Using yourls__ for text string now
oops, missing closing parenthesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find any .po or .mo files in the project. I guess this is just "prepared" for l10n, but there's only one language as of now? Or maybe I'm missing something..
Well, I inserted that one line I changed here into my local installation via VI and it works :) Take it or leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, never noticed.
The DB is initialized with a couple example links, so someone has to intentionally bring a new install down to zero stored items in order to see this :)
I cannot find any .po or .mo files in the project. I guess this is just "prepared" for l10n, but there's only one language as of now? Or maybe I'm missing something..
A .pot
is maintained automatically in a separate repo: https://github.com/YOURLS/YOURLS.pot
You are correct that YOURLS itself contains only English localization. Translations based on the .pot
are submitted by their maintainers to be listed on the Awesome List: https://github.com/YOURLS/awesome#translations
Tab! Apply directly to the SPACES! Also more nuanced logic: only show "counting _n_ clicks" if there are more than 0 items.
You know, I was doing my level best to make a code review suggestion that would tweak all the style points I wanted to hit (tabs instead of spaces, whitespace around parameters, our little conventions) and then realized I should test the thing. Testing (live, in production 😱) got me thinking about more nuanced logic for display of different message parts, so I hope you don't mind that I just ended up authoring a "code review commit" directly applying the changes I'd make. Try it out! I think the output works very nicely in all cases.* * — I didn't delete all URLs from my instance to test that case, but I hit "search with results", "search with 0 results", and the default list without any filters. |
Sorry about the tabs and spaces, I had no idea you feel so strongly about it. You see I don't have an IDE at the moment. I edited the index.php on my server with VI, then copy pasted from the bash console into GitHub. No wonder the tabs got messed up. As for testing, you don't need to remove your urls. Just put $total_items = 0 before the if and you're good to test, even on a live system, because it doesn't affect the redirection functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will let another maintainer look this over (including my tweaks).
Just put $total_items = 0 before the if and you're good to test
Ah of course, I'm clearly not in "webapp brain" today. That case is now tested too.
You see I don't have an IDE at the moment. I edited the index.php on my server with VI, then copy pasted from the bash console into GitHub. No wonder the tabs got messed up.
Funny enough, I was initially making tweaks on my server in VI. No, I don't trust its automatic tabs/spaces; final patch was checked in VS Code locally before committing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite a tiny tweak, lgtm
Co-authored-by: Léo Colombaro <LeoColomb@users.noreply.github.com>
You know how it is - you got something perfect, a bleach-white shirt, and people will notice the tiniest speck. This one fixes the message "Display 1 to 0 of 0 URLs." on a new installation. Best regards from your friendly autist. ;-)