<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4201#discussion_r1310163561">app/helpers/issues_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -26,9 +26,9 @@ def reportable_title(reportable)
end
def open_issues_count
- count = Issue.visible_to(current_user).open.limit(100).size
- if count > 99
- tag.span("99+", :class => "count-number")
+ count = Issue.visible_to(current_user).open.limit(Settings.max_issues_count).size
</pre>
<p dir="auto">So I think this will all be more straightforward if the setting is the same as the digits shown. So if you want to show <code class="notranslate">99+</code> you set the limit to <code class="notranslate">99</code>. It's strange to set the limit to "100" in then a different number to show up.</p>
<p dir="auto">Additionally, 99+ just means "at least" 99, so it's a valid display for both 99 and 100. The original implementation checks for <code class="notranslate">99+1</code> and then shows either <code class="notranslate">99</code> or <code class="notranslate">99+</code>, but this is unnecessary complexity. Better just to go <code class="notranslate">97</code>, <code class="notranslate">98</code>, <code class="notranslate">99+</code>, and this means no +1 maths is necessary.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4201#discussion_r1310176384">app/helpers/issues_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -26,9 +26,9 @@ def reportable_title(reportable)
end
def open_issues_count
- count = Issue.visible_to(current_user).open.limit(100).size
- if count > 99
- tag.span("99+", :class => "count-number")
+ count = Issue.visible_to(current_user).open.limit(Settings.max_issues_count).size
+ if count >= Settings.max_issues_count
+ tag.span("#{Settings.max_issues_count - 1}+", :class => "count-number")
</pre>
<p dir="auto">This is an i18n issue, since not all languages use the <code class="notranslate">NN+</code> format. So it should be moved to a translatable string.</p>
<p dir="auto">I investigated CLDR to get the right terminology here, which is either 'at_least_pattern' or 'atLeast' depending on which be of CLDR you look at. So having that terminology in the key name will be helpful for translators.</p>
<p dir="auto"><a href="https://github.com/search?q=repo%3Aunicode-org%2Fcldr-json%20atleast&type=code">https://github.com/search?q=repo%3Aunicode-org%2Fcldr-json%20atleast&type=code</a></p>
<p dir="auto">(I have researched if there is something equivalent available in rails-i18n, but with no success. It would be more useful to use CLDR as the source for this, but more hassle than it's worth. But at least we can let our own translators deal with it).</p>
<p dir="auto">Finally it should be a reusable translation (i.e. not controller-specific), since we'll end up using it for other things like message inbox counts too (which are currently an unlimited <code class="notranslate">count</code>, and therefore could be a performance issue).</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4201#pullrequestreview-1602546465">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLNX6PH5A4VGGYICY53XX4VODANCNFSM6AAAAAA35HEQBE">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLOKCJMKIIBHBZZBDTDXX4VODA5CNFSM6AAAAAA35HEQBGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTS7QTVSC.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4201/review/1602546465</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4201#pullrequestreview-1602546465",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4201#pullrequestreview-1602546465",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>