Opened 11 years ago
Closed 11 years ago
#20246 closed Cleanup/optimization (fixed)
Use non-breakable space between amount and units
Reported by: | Vlastimil Zíma | Owned by: | EmilStenstrom |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | dceu13 |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Template filters which returns units should return strings with non-breakable space '\xa0'
between amount and units. It increases legibility.
Affected filters:
filesizeformat
timesince
timeuntil
Reference:
Change History (11)
comment:1 Changed 11 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 11 years ago by
Owner: | changed from nobody to EmilStenstrom |
---|---|
Status: | new → assigned |
comment:3 Changed 11 years ago by
Has patch: | set |
---|
Here's a pull request:
https://github.com/django/django/pull/1106
Things I've thought about:
- I replace on the value instead of inserting "\xa0" manually into the translation strings. This avoids having to change all of the translation strings.
- I don't insert a non-breaking space for composites of several values, ie. "1<nbsp>hour, 2<nbsp>minutes"
- Humanize uses timesince in some cases, so some testcases failed. But fixing all of humanize is a lot of work, so I suggest that is broken out into a separate ticket. It will require breaking up transation strings is smaller units, and retranslating them. I have only fixed the tests that prevented the tests from passing.
Does this change need documentation?
comment:4 Changed 11 years ago by
There's two things that I'm uncertain on:
1) Breaking Humanize is obviously a big task for the benefit of fixing this little thing.
2) Making it the default and only rendering behaviour. I know people that like having the multiline approach and this potentially makes it very hard to do.
comment:5 Changed 11 years ago by
@martmatwarne:
1) This patch does not break Humanize, all tests continue passing after this fix. The only thing that changes after this patch is that some of the humanize cases have a \xa0 inserted, but not all of them. I suggest a new ticket for making all of them get proper non-breaking spaces.
2) I think wanting to disable this should be fairly rare. You could easily fix it by writing your own filter when you run timesince, and replace "\xa0" with " " and get back the old behaviour. My assumption is that this is something very few people want.
comment:6 Changed 11 years ago by
Keywords: | dceu13 added |
---|---|
Patch needs improvement: | set |
To clarify: humanize isn't broken with this patch: it's behaviour becomes inconsistent, as it calls timesince for some timeframes, and has it's own code for others. Fixing this all through humanize is very painful, so my recommendation is to give utils.timesince an optional argument like avoid_wrapping=True
, so that humanize can use that to never get the non-breaking spaces. This is not ideal, and we should fix humanize for this one day, but at least it means the same template tag always shows the same behaviour.
Other than that, it would be good if the tests have a comment about what \xa0
is supposed to mean, as that is otherwise not obvious.
comment:7 Changed 11 years ago by
@erikr: Thanks for the review! I've updated the patch with the two suggestions you had:
1) Humanize now calls timesince with avoid_wrapping=False. I have reverted all humanize tests, so they are untouched.
2) I've added a comment in the three test cases that explains what \xa0 means. The comment only appears the first time it's used, to keep the duplication down.
comment:8 Changed 11 years ago by
I'd rather fix humanize and add the \xa0
inside translatable strings when we cannot avoid it (e.g. '%(count)s\xa0seconds ago'
. Either we fix this everywhere or not at all.
comment:9 Changed 11 years ago by
@claudep: I've fixed humanize by changing the translation string, and removed the parameter to timesince. Any other outstanding issues?
comment:10 Changed 11 years ago by
More updates to the pull request: I've change from using \xa0 to using \u00a0 because makemessages didn't work otherwise. I've also added translator comments to the changed translation strings to make it easier for translators to understand what's going on.
I feel this is ready to commit now.
comment:11 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
At first glance, I'd be in favour of that suggestion.