Opened 4 years ago

Closed 2 years ago

#16350 closed Bug (fixed)

Admin messages should be capitalized to work well in other languages

Reported by: semente 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 semente 4 years ago.
design-language-issue.diff (452 bytes) - added by semente 4 years ago.
Screen shot 2011-06-26 at 21.45.47.png (11.3 KB) - added by aaugustin 4 years ago.
Screenshot-with-original-patch-in-Google-Chrome.png
16350-fix-in-css.diff (660 bytes) - added by aaugustin 4 years ago.
16350-fix-in-template.diff (662 bytes) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (29)

Changed 4 years ago by semente

Changed 4 years ago by semente

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 4 years ago by aaugustin

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

comment:2 Changed 4 years ago by aaugustin

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

Changed 4 years ago by aaugustin

Changed 4 years ago by aaugustin

comment:3 Changed 4 years ago by semente

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 follow-up: Changed 4 years ago by julien

  • Needs tests set

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

comment:5 follow-up: Changed 4 years ago by jezdez

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

comment:6 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by aaugustin

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 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to 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 in reply to: ↑ 6 Changed 4 years ago by julien

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 Changed 4 years ago by aaugustin

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 Changed 4 years ago by julien

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 in reply to: ↑ 5 Changed 4 years ago by semente

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 Changed 4 years ago by lukeplant

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 Changed 4 years ago by julien

  • 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 Changed 4 years ago by BernhardEssl

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 Changed 4 years ago by julien

#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 Changed 4 years ago by julien

  • Summary changed from Small admin CSS issue: messagelist items should be capitalized to work well in other languages to Admin messages should be capitalized to work well in other languages
  • Triage Stage changed from Design decision needed to Accepted

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

comment:17 Changed 4 years ago by jezdez

  • Triage Stage changed from Accepted to Design decision needed

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

comment:18 Changed 4 years ago by erw

  • 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 Changed 4 years ago by julien

Note that the admin already uses capfirst in many places, especially to capitalize the verbose model and field names (which are translatable).

comment:20 Changed 4 years ago by erw

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 Changed 4 years ago by erw

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 Changed 4 years ago by aaugustin

  • Patch needs improvement set

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

comment:23 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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

comment:24 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top