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

Fix pluralization arguments for admin list summaries #14611

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

thatplatypus
Copy link
Contributor

@thatplatypus thatplatypus commented Oct 31, 2023

While using the admin portion of Orchard Core, we had noticed there were a couple places where the pluralization seems to be interpreted incorrectly and get displayed as OrchardCore.Localization.PluralizationArgument instead of actually replacing it with the 1 or actual amount. Testing out these changes locally, it seems as though it just does not like the {0} arguments twice.

image

image

@thatplatypus
Copy link
Contributor Author

@dotnet-policy-service agree

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and I can't seem to be able to reproduce your original issue.

With the code on the latest main, it works properly for me with 0, 1, and more items, both the content list and the Audit Trail:

image

Could you check again, please?

@hishamco
Copy link
Member

You're right @Piedone I think we need to revise the old PRs some of them were created years ago, and they have been ignored accidentally over time

IMHO we need to put some effort into the old PRs

@thatplatypus
Copy link
Contributor Author

Good morning @Piedone and @hishamco

Yes I think you're correct. It's been a while but I think after raising this we ended up tracing the root issue back to it either being a mistake in our tenant setup, or something with our DI wired up incorrectly. I can't quite remember, but we aren't seeing the issue anymore.

We were on 1.5, but confirmed on both 1.5 and 1.8 that it is working.

Given that I'm not sure there's a ton of value in my change since it's just style at that point. I imagine singular parameterized 1 should in practice always be 1 like the documentation example?

Either way, feel free to reject this or let me know if I should do anything else.

Thank you both for your work on the project! We actually just went to prod with Orchard in our backend so that was pretty exciting.

Cheers

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Thanks for your quick reply, awesome! Actually indeed, semantically it's better to have the singular version refer to 1. I'm merging this, thanks again!

@Piedone Piedone merged commit d623f11 into OrchardCMS:main Jan 10, 2024
3 checks passed
@hishamco
Copy link
Member

Thanks for your contribution @thatplatypus

hishamco pushed a commit that referenced this pull request Feb 1, 2024
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
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.

3 participants