Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MNTOR-1809 - Add chart into dashboard top banner #3232
MNTOR-1809 - Add chart into dashboard top banner #3232
Changes from all commits
2a6ae97
ee4c446
2115741
9cd205f
876bd1d
7c63c22
2330cc5
f6fadea
e8c6ca5
b1438b5
a90adc4
4efb33c
3b27dcc
30862d5
1d593d6
1215501
919f46b
014e567
f8afc68
04dd956
f7aac09
d7844bd
58d16f5
d614a31
c7c4355
2dce8fc
ebfcd2c
e8704b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@codemist
I missed this one in review (not sure if it was there when I reviewed, but that's not relevant).
All categories in this file are plurals. Also, why is this category being added in this PR? Just a coincidence?
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.
@flodolo Me thinks it's related to this: #3244 (comment)
We have a bunch of these breach categories in the new dashboard UI now, so rather than add duplicate strings for localizers to localize, we reused the existing translated strings in locales/*/data-classes.ftl (although yes, that does slightly blur the fact that 99.999% of the values in that file are HIBP specific except this one).
And yes, good callout that now there is some vague pluralizing where the ID is plural and the value is singular.
Not sure which way to fix that one, or if you think we should remove "full-names" entirely and move it into one of the files in /locales-pending/*.ftl. Plural would be more consistent with the rest of the values in data-classes.ftl, but I'm not clear on if that'd make the dashboard UI less clear, so I defer to UX/content/@codemist.
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.
Reusing strings is a bad idea, especially if we mix them from different sources. Some locales, for example, are translating categories lowercase.
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.
It's less than ideal in terms of localizability, but one way to minimize the impact would be to capitalize the first letter of the category (using a locale-sensitive library) when used in a different context.