Opened 13 years ago
Closed 12 years ago
#16350 closed Bug (fixed)
Admin messages should be capitalized to work well in other languages
| Reported by: | Guilherme Gondim | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.admin | Version: | 1.3 |
| Severity: | Normal | Keywords: | css, admin, pt-br |
| Cc: | erik.wognsen@… | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | yes | UI/UX: | yes |
Description
Admin's messagelist items should be capitalized. When using the pt-br localization the first letter on the messages is not capitalized because it is the object name, who can't be capitalized in the POT files.
I've attached a screenshot to you figure out better the problem.
Attachments (5)
Change History (29)
by , 13 years ago
| Attachment: | Screenshot.png added |
|---|
by , 13 years ago
| Attachment: | design-language-issue.diff added |
|---|
comment:1 by , 13 years ago
| Needs documentation: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
by , 13 years ago
| Attachment: | Screen shot 2011-06-26 at 21.45.47.png added |
|---|
Screenshot-with-original-patch-in-Google-Chrome.png
comment:2 by , 13 years ago
| Easy pickings: | set |
|---|---|
| Needs documentation: | unset |
The CSS spec confirms that text-transform: capitalize; isn't appropriate here: http://www.w3.org/TR/2008/REC-CSS1-20080411/#text-transform
I attached two new patches:
- the first one fixes the problem in CSS — but that doesn't feel very Django-like;
- the second one fixes the problem in the template — that's better IMO.
(and I wanted to set "needs improvement", not "needs documentation" earlier — fat fingers)
by , 13 years ago
| Attachment: | 16350-fix-in-css.diff added |
|---|
by , 13 years ago
| Attachment: | 16350-fix-in-template.diff added |
|---|
comment:3 by , 13 years ago
Hello aagustin, sorry for the mistakes in the patch!
Django capitalize some letters in CSS or template, so maybe we should use the one that is used more often.
Anyway, technically speaking, fix that kind of bug in the template is better (IMHO). To a user change that behavior in CSS him must provide a new CSS file (and add it in the <head>) or overwrite the whole CSS. So usually will be simpler if him just overwrite the template block.
follow-up: 6 comment:4 by , 13 years ago
| Needs tests: | set |
|---|
That's right, using capfirst in the template is the way to go. This now needs tests :)
follow-up: 11 comment:5 by , 13 years ago
Hmm, is it really valid for all languages to capitalize the first word?
follow-up: 8 comment:6 by , 13 years ago
Replying to julien:
That's right, using
capfirstin the template is the way to go. This now needs tests :)
I'm curious to know how many of the 33 occurrences of capfirst in the admin are actually tested by the test suite :(
You'd better close the ticket as 'wontfix'. It's more realistic than hoping that someone's going to write tests for this.
comment:7 by , 13 years ago
| Triage Stage: | Accepted → Design decision needed |
|---|
Replying to jezdez:
Hmm, is it really valid for all languages to capitalize the first word?
That's a good point, yet the admin already capitalizes the first word of some translatable strings (e.g. verbose_name_plural in delete_selected_confirmation.html). Moving to DDN until a clear answer can be provided on the best approach to handle this.
comment:8 by , 13 years ago
Replying to aaugustin:
I'm curious to know how many of the 33 occurrences of capfirst in the admin are actually tested by the test suite :(
You'd better close the ticket as 'wontfix'. It's more realistic than hoping that someone's going to write tests for this.
Only this one would need to be tested to proceed with this particular ticket. If other cases were tested then that'd be even better, but it's not necessary here. It's important to write tests even for seemingly small changes like this one to avoid future regressions.
comment:9 by , 13 years ago
Sorry for the angry tone in my reply. I wanted to say that, in this particular case, the cost of writing a test is very high compared to the importance of the bugfix: an utterly minor aesthetic tweak.
I know the theory of testing, and I agree that tests are important — just look at the number of tickets where I set "needs tests". But we have to be realistic: if a good Django hacker spends 30 minutes to write a test for this, it's a waste of time. There are much more important bugs to fix in the admin.
In short, IMO, we're not focusing on the right priorities. It's OK if you disagree and that's why I won't toggle the flag. Now let's stop hijacking the tracker with our private discussions :)
comment:10 by , 13 years ago
No problem, I understand the frustration. Been there :-)
Consistency in requiring tests for (nearly) everything is actually as important as the tests themselves, as it's always going to be subjective and debatable what is or is not big enough to require tests. Also, sometimes, small and seemingly harmless changes end up causing serious regressions. It is true that there are more important problems to solve in the admin, but everyone is free to spend their time working on the bits they want -- it's never going to be a waste of time. If someone cares enough about this problem then they'll fix it, write tests and fullfil all the requirements for this ticket to proceed.
It's definitely OK to disagree, and please don't just take my word for it and don't let this disagreement hamper your contributions which are very much appreciated! :-)
comment:11 by , 13 years ago
Replying to jezdez:
Hmm, is it really valid for all languages to capitalize the first word?
I'm almost sure that it is valid for all latin-based languages, but has other languages affected by this patch (you can see which one using grep) so maybe we should send that question to the i18n list and listen for opinions from people from that languages.
About the test, I think is valid create a test for the capfirst filter but not for that patch.
comment:12 by , 13 years ago
I'd be happy to accept the change without a test. When it comes to testing specific output in HTML, the test can be extremely difficult to write in such a way that it really tests the issue, and even if you manage it, it can be so fragile that it is a liability not an asset - e.g. if you rearrange the HTML in the admin, the test will break (either by failing or by returning a false positive that just causes confusion later).
I have seen tests written for very specific HTML output, and have often concluded that they were worse than useless.
comment:13 by , 13 years ago
| Needs tests: | unset |
|---|
That's fair enough, it's debatable and I'm clearly in the minority, so I'm removing the flag. Thanks guys for sharing your thoughts!
comment:14 by , 13 years ago
Sorry, but I don't think that's a real problem. Why not use in your model the Meta class with verbose_name?
comment:15 by , 13 years ago
#16973 was closed as a dupe and has a different patch. See also the discussion on django-i18n: http://groups.google.com/group/django-i18n/browse_thread/thread/61c19470026d5a83
comment:16 by , 13 years ago
| Summary: | Small admin CSS issue: messagelist items should be capitalized to work well in other languages → Admin messages should be capitalized to work well in other languages |
|---|---|
| Triage Stage: | Design decision needed → Accepted |
Also marking as Accepted and changing the description since the approach that's been agreed upon is to use capfirst.
comment:17 by , 13 years ago
| Triage Stage: | Accepted → Design decision needed |
|---|
I haven't seen a response from language communities other than latin-based, so marking as DDN again.
comment:18 by , 13 years ago
| Cc: | added |
|---|
Another question to consider is whether it is ok to capfirst all messages, even in latin writing systems? I think it is a bit intrusive to force capfirst. My patch in #16973 only capfirsts some relevant admin messages.
comment:19 by , 13 years ago
Note that the admin already uses capfirst in many places, especially to capitalize the verbose model and field names (which are translatable).
comment:20 by , 13 years ago
Sorry, I was thinking of projects that customize the admin where you might want more control over custom messages, but if you're doing that, you could also customize the messages block in admin/base.html anyway. So never mind my comment above.
comment:21 by , 13 years ago
Replying to jezdez:
I haven't seen a response from language communities other than latin-based, so marking as DDN again.
Since there is still no reply from a non-latin based language communities (neither here nor in the i18n group) and especially, as julien points out, capfirst is already used in many places, I think the catch-all capfirst in the fix-in-template patch should be accepted. Also, as Sergiy points out in the i18n discussion (1), uppercasing is a no-op for (at least some) non-latin writing systems.
(1): http://groups.google.com/group/django-i18n/browse_thread/thread/61c19470026d5a83
comment:22 by , 13 years ago
| Patch needs improvement: | set |
|---|
comment:23 by , 12 years ago
| Triage Stage: | Design decision needed → Accepted |
|---|
Let's go with capfirst in the template, we've waited more than enough for opinions on this minuscule detail.
comment:24 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Fixed in f7d945e325fd8edd0d827e652fb410bb2589de02.
Patch doesn't apply cleanly — it wasn't created against SVN trunk. Also, capitalize isn't appropriate as it will capitalize each word (see attached screenshot).