MBS-8748: Wrap medium name in release tracklist #481
Conversation
yvanzo
force-pushed
the
mbs-8748-wrap-medium-header
branch
from
212f013
to
3f1625d
Compare
root/components/common-macros.tt
Outdated
| @@ -834,9 +834,9 @@ END -%] | |||
| }; | |||
| IF medium.name != ''; | |||
| args.medium_name = isolate_text(medium.name); | |||
| l('{medium_format} {position}: {medium_name}', args); | |||
| l('<span>{medium_format} {position}:</span> {medium_name}', args); | |||
There was a problem hiding this comment.
I think it'd be good to add something like class="medium-format-position" here and refer to that in the CSS, so we don't have to worry about ambiguity later. Also easier to grep where a style is coming from that way.
There was a problem hiding this comment.
There is no specific rule for this element in the style sheet as > span rules include span.expand-triangle also. Instead, I could wrap {medium_name} into a span with medium-name class to refer to in the style sheet in place of not-semantic bdi. Or do both?
There was a problem hiding this comment.
Right, though I think it's good to avoid CSS selectors that depend too much on the structure of the HTML. Unless the style would break if it wasn't a direct child of <a>.
About ambiguity, I mean if we change the surrounding HTML later and the generic <span> no longer works, we'll have to change the string again (unless it has a class name we could use).
Feel free to change it to whatever you think is best (or we can merge it as-is).
There was a problem hiding this comment.
I cleaned CSS rules and reduced changes made to localized messages in updated 83e0fc4
I added medium-name class only which is useful to apply normal white-space wrapping style .
Other classes can be added later on if necessary without changing localized messages.
yvanzo
force-pushed
the
mbs-8748-wrap-medium-header
branch
from
3f1625d
to
83e0fc4
Compare
root/components/common-macros.tt
Outdated
| args.medium_name = isolate_text(medium.name); | ||
| l('{medium_format} {position}: {medium_name}', args); | ||
| l('{medium_format_position} {medium_name}', { | ||
| medium_format_position => '<span>' _ add_colon(format_position) _ '</span>', |
There was a problem hiding this comment.
I like how the HTML was moved out of the string. 👍
I guess this'll allow translators to change the order of {medium_format_position} and {medium_name}, and maybe add additional words for context. Does that make sense if these are supposed to be in separate cells?
There seems to be a contradiction between having these cells visually separated from each other, but allowing translators to edit the string as if it's all in one cell. So maybe we shouldn't have this "combined" string anymore?
There was a problem hiding this comment.
It makes sense for right-to-left locales such as Arabic, although Arabic translation is currently 0% complete and most of UI (except medium header) is not ready for right-to-left locales yet. 😆
But I agree, left-to-right ordering of cells would have to be done by template, not by message.
Updated in bc878ed
Long medium names caused horizontal scrolling on release pages. This patch wraps medium names and only medium names. In order to keep distinguishing medium format and position from medium name, header link is split into elements which are styled as table cells. The ordering of these cells is moved from message to template level. Template macro `medium_description` has been modified so that `span`/`class` can be added without change to localizable messages.
yvanzo
force-pushed
the
mbs-8748-wrap-medium-header
branch
from
83e0fc4
to
bc878ed
Compare
MBS-8748: Long medium names caused horizontal scrolling on release pages.
This patch wraps medium name and visually split the header line into (borderless) cells so that medium format and medium number are clearly distinguished from wrapped medium name.