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 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

At first glance, I'd be in favour of that suggestion.

comment:2 by EmilStenstrom, 11 years ago

Owner: changed from nobody to EmilStenstrom
Status: newassigned

comment:3 by EmilStenstrom, 11 years ago

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 by martmatwarne, 11 years ago

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 by EmilStenstrom, 11 years ago

@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 by Sasha Romijn, 11 years ago

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 by EmilStenstrom, 11 years ago

@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 by Claude Paroz, 11 years ago

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 by EmilStenstrom, 11 years ago

@claudep: I've fixed humanize by changing the translation string, and removed the parameter to timesince. Any other outstanding issues?

https://github.com/django/django/pull/1106/files

comment:10 by EmilStenstrom, 11 years ago

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 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 7d77e9786a118dd95a268872dd9d36664066b96a:

Fixed #20246 -- Added non-breaking spaces between values an units

Note: See TracTickets for help on using tickets.
Back to Top