8000 Fix "Display 1 to 0 of 0 URLs" on admin list page by SimStim · Pull Request #3910 · YOURLS/YOURLS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 6 commits into from
May 1, 2025
Merged

Conversation

SimStim
Copy link
Contributor
@SimStim SimStim commented Apr 28, 2025

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. ;-)

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. ;-)
@ozh
Copy link
Member
ozh commented Apr 28, 2025

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

@SimStim
Copy link
Contributor Author
SimStim commented Apr 28, 2025

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!

SimStim added 2 commits April 28, 2025 20:08
Using yourls__ for text string now
oops, missing closing parenthesis
Copy link
Contributor Author
@SimStim SimStim left a 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.

Copy link
Member
@dgw dgw left a 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

SimStim and others added 2 commits April 30, 2025 13:12
Message on zero URLs has been made more universal: - If no URLs in database, "No URLs." - If no URLs found while searching with filters, an additional recommendation to be less specific is added. Later it still outputs ", counting 0 clicks." - which I don't consider helpful in this particular case. But let's not lump everything into one big pull request. And the "proper" behaviour is not for me to decide anyway. So I think, my original idea "is done here."
Tab! Apply directly to the SPACES!

Also more nuanced logic: only show "counting _n_ clicks" if there are
more than 0 items.
@dgw
Copy link
Member
dgw commented Apr 30, 2025

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.

@dgw dgw changed the title Update index.php Fix "Display 1 to 0 of 0 URLs" on admin list page Apr 30, 2025
@SimStim
Copy link
Contributor Author
SimStim commented Apr 30, 2025

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.

Copy link
Member
@dgw dgw left a 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.

Copy link
Member
@LeoColomb LeoColomb left a 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>
@dgw dgw enabled auto-merge (squash) May 1, 2025 23:01
@dgw dgw merged commit a6b0f07 into YOURLS:master May 1, 2025
6 checks passed
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.

4 participants
0