Closed
Bug 922576
Opened 11 years ago
Closed 11 years ago
Allow reordering HTML elements in translations
Categories
(L20n :: HTML Bindings, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: stas, Assigned: stas)
References
Details
Attachments
(1 file)
11.60 KB,
patch
|
zbraniecki
:
review+
mgol
:
feedback+
|
Details | Diff | Splinter Review |
The work in bug 921169 made it impossible (or risky) to reorder elements in the translations. We should consider allowing (from easiest to most difficult): 1. reordering elements of different type: input a → a input, 2. reordering elements of same type with an id: a#one a#two → a#two a#one (this introduces a dependency on developers who need to add ids), 3. reordering elements of same type without additional work from developers: a a → a#l10n-path=a[2] a#l10n-path=a[1]
Comment 1•11 years ago
|
||
Can't it be done using `insertBefore`?
Assignee | ||
Comment 2•11 years ago
|
||
We already use insertBefore. The problem is that I want to identify elements that need not be replaced, but also insert new elements. Right now, I do this sequentially: start from the first child in the source, find its equivalent in the translation, take all nodes up that equivalent from the translation and move them into source. If source has two elements (input a), I'll start with input, find it in the translation (where the order is reversed: a input), take all nodes preceding input (a) and move them into source. Then wehn I move on to 'a' in source, I don't find it in the translation anymore. So doing this sequentially is limiting. An alternative would be to sanitize translation's DOM first and then use translation.replaceChild to move the source element from source DOM to translation overlay it there, and then move the whole translation back into source.
Assignee | ||
Comment 3•11 years ago
|
||
This patch implements the approach from comment 2 in order to allow the first type of the reordering mentioned in comment 0. I should have done this like this from the beginning, it's much simpler and just as fast. Settings, l20n-master, languages_dev, 30 iterations: master: 1139 patch: 1143
Attachment #813869 -
Flags: review?(gandalf)
Comment 4•11 years ago
|
||
Comment on attachment 813869 [details] [diff] [review] Allow reordering Review of attachment 813869 [details] [diff] [review]: ----------------------------------------------------------------- sweet work Stas!
Attachment #813869 -
Flags: review?(gandalf) → review+
Updated•11 years ago
|
Assignee: nobody → stas
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 813869 [details] [diff] [review] Allow reordering Michał, would you mind taking a look and seeing if this still works with Angular?
Attachment #813869 -
Flags: feedback?(m.goleb+mozilla)
Comment 6•11 years ago
|
||
Stas: your patch generates a massive amount of errors to the console in Polona. There are two kinds of errors, here are tracebacks: http://pastebin.com/z5WSMMQq
Assignee | ||
Comment 7•11 years ago
|
||
Michał: are these really related to my patch? I think they might be caused by the bug we discussed on IRC last week about _source being null instead of '' when the resource download fails (I filed bug 924071 for this).
Flags: needinfo?(m.goleb+mozilla)
Comment 8•11 years ago
|
||
(In reply to Staś Małolepszy :stas from comment #7) > Michał: are these really related to my patch? I think they might be caused > by the bug we discussed on IRC last week about _source being null instead of > '' when the resource download fails (I filed bug 924071 for this). That might be true, your patch might have just made these bugs more explicit. However, that's what I see after applying your patch, I'm not saying it's your fault, I only said what happened. ;)
Flags: needinfo?(m.goleb+mozilla)
Comment 9•11 years ago
|
||
Apart from these errors, it seems to work fine.
Comment 10•11 years ago
|
||
@stas Ah, sorry, those errors are already present on master, some other changes had to introduce them. :/
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Staś Małolepszy :stas from comment #0) > 1. reordering elements of different type: input a → a input, Fixed in https://github.com/l20n/l20n.js/commit/97ecf63377a789aeaff2f08b3e366fbbc47c4fd6 > 2. reordering elements of same type with an id: a#one a#two → a#two a#one > (this introduces a dependency on developers who need to add ids), > 3. reordering elements of same type without additional work from > developers: a a → a#l10n-path=a[2] a#l10n-path=a[1] If we get to implementing #2 and #3, we should file new bugs.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment #813869 -
Flags: feedback?(m.goleb+mozilla) → feedback+
You need to log in
before you can comment on or make changes to this bug.
Description
•