Code

#20246 closed Cleanup/optimization (fixed)

Use non-breakable space between amount and units

Reported by: vzima Owned by: EmilStenstrom
Component: Template system Version: master
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:

Attachments (0)

Change History (11)

comment:1 Changed 12 months ago by claudep

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

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

comment:2 Changed 11 months ago by EmilStenstrom

  • Owner changed from nobody to EmilStenstrom
  • Status changed from new to assigned

comment:3 Changed 11 months ago by EmilStenstrom

  • 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 months ago by martmatwarne

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 months ago by EmilStenstrom

@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 months ago by erikr

  • 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 months ago by EmilStenstrom

@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 months ago by claudep

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 months ago by EmilStenstrom

@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 Changed 11 months ago by EmilStenstrom

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 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 7d77e9786a118dd95a268872dd9d36664066b96a:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.