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)

Screenshot.png (5.9 KB ) - added by Guilherme Gondim 13 years ago.
design-language-issue.diff (452 bytes ) - added by Guilherme Gondim 13 years ago.
Screen shot 2011-06-26 at 21.45.47.png (11.3 KB ) - added by Aymeric Augustin 13 years ago.
Screenshot-with-original-patch-in-Google-Chrome.png
16350-fix-in-css.diff (660 bytes ) - added by Aymeric Augustin 13 years ago.
16350-fix-in-template.diff (662 bytes ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (29)

by Guilherme Gondim, 13 years ago

Attachment: Screenshot.png added

by Guilherme Gondim, 13 years ago

Attachment: design-language-issue.diff added

comment:1 by Aymeric Augustin, 13 years ago

Needs documentation: set
Triage Stage: UnreviewedAccepted

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

by Aymeric Augustin, 13 years ago

Screenshot-with-original-patch-in-Google-Chrome.png

comment:2 by Aymeric Augustin, 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 Aymeric Augustin, 13 years ago

Attachment: 16350-fix-in-css.diff added

by Aymeric Augustin, 13 years ago

Attachment: 16350-fix-in-template.diff added

comment:3 by Guilherme Gondim, 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.

comment:4 by Julien Phalip, 13 years ago

Needs tests: set

That's right, using capfirst in the template is the way to go. This now needs tests :)

comment:5 by Jannis Leidel, 13 years ago

Hmm, is it really valid for all languages to capitalize the first word?

in reply to:  4 ; comment:6 by Aymeric Augustin, 13 years ago

Replying to julien:

That's right, using capfirst in 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 Julien Phalip, 13 years ago

Triage Stage: AcceptedDesign 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.

in reply to:  6 comment:8 by Julien Phalip, 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 Aymeric Augustin, 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 Julien Phalip, 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! :-)

in reply to:  5 comment:11 by Guilherme Gondim, 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 Luke Plant, 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 Julien Phalip, 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 Bernhard Essl, 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 Julien Phalip, 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 Julien Phalip, 13 years ago

Summary: Small admin CSS issue: messagelist items should be capitalized to work well in other languagesAdmin messages should be capitalized to work well in other languages
Triage Stage: Design decision neededAccepted

Also marking as Accepted and changing the description since the approach that's been agreed upon is to use capfirst.

comment:17 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedDesign decision needed

I haven't seen a response from language communities other than latin-based, so marking as DDN again.

comment:18 by Erik Wognsen, 13 years ago

Cc: erik.wognsen@… 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 Julien Phalip, 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 Erik Wognsen, 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 Erik Wognsen, 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 Aymeric Augustin, 13 years ago

Patch needs improvement: set

#16973 and #17246 were merged into this ticket, but the current patch doesn't address them.

comment:23 by Aymeric Augustin, 12 years ago

Triage Stage: Design decision neededAccepted

Let's go with capfirst in the template, we've waited more than enough for opinions on this minuscule detail.

comment:24 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top