In the field formatter, when the days are 'grouped', the label width the same as 'not grouped' and is to small.
for example, the space for 'Wed:' and 'Wed-Fri:' is the same.
The width of the 'dayname' column is calculated in file office_hours.theme.inc .
This calculation is moved to another spot with the fix in comment #1.
Comment #5:
"We can fix this by creating a 'real' table. ATM you can set ' < br >' as a separator between days.
Depending on a special separator (for example, '< table >', or just the existing '< br >') we can format a true table, which makes the label-width discussion obsolete.
Comment #6:
"A table would solve the issue, but will break backward compatibility with existing themes. We could implement a separate formatter theme that displays a table instead of a div's. Should even be possible to reuse most of the existing refactored code.
Comment #8:
Attached patch #8 generates a proper html-table, which set the width's automatically. It's not ideal yet, since the background is not transparent, giving 'squares' in e.g. Views lists.
"A table would solve the issue, but will break backward compatibility with existing themes.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | office_hours_1614206_8_label_width.patch | 3.83 KB | johnv |
Comments
Comment #1
johnvFixed in 1fe0773
There is some dispute wether this css should be in the code.
For now, I centralized the calculation in 1 place, and corrected the error.
style="width: ' . ($max_label_length * 0.70) . 'em; "IMO a fixed width cannot accomodate for different translations and does not work automatically when you use short and long daynames in differnte places of your site.
An alternative would be to create a setting 'label width = fixed/calculated/css'.
Although committed to code, status is set to 'needs review', as in 'open for discussion'.
Comment #2
Lazarus-Long commentedI'm still very against using inline styles to format the text. It's very difficult for the designer to override it if they want to, and it mixes up the presentation and styling layers.
Side note, // comments aren't valid in CSS. /* */ are.
Comment #3
johnvI've tested with IE8 and Chrome, and it works fine.
On the web, indeed only /**/ is shown on several documentation pages.
So it only works in 'modern' browsers?
Comment #4
Lazarus-Long commentedMost browsers will accept // since it's so common, but it's not part of the standard and the CSS would fail validation.
Comment #5
johnvIMO we can fix this by creating a 'real' table. ATM you can set ' < br >' as a separator between days.
Depending on a special separator (for example, '< table >', or just the existing '< br >') we can format a true table, which makes the lalbe-width discussion obsolete.
Any ideas?
Comment #6
Lazarus-Long commentedA table would solve the issue, but will break backward compatibility with existing themes. We could implement a separate formatter theme that displays a table instead of a div's. Should even be possible to reuse most of the existing refactored code.
Comment #7
johnvThat's why I wanted to introduce the '< table >' keyword. For new installations it will be the default. Existing installations, will retain the current ' < br> ' default as a line separator.
Comment #8
johnvAttached patch generates a proper html-table, which set the width's automatically.
it's not ideal yet, since the background is not transparent, giving 'squares' in e.g. Views lists.
Comment #10
Anonymous (not verified) commentedCode from this patch seems to be in stable version of Office Hours module. It sends a fixed width into the template right from the code. There might be a use case for some people but for my project this forced me to remove code from the module. CSS should be where it belongs: in some css file!
Comment #11
johnv@sint, I have updated the issue summary. I don't know why fix #1 is automatically applied in D8, 2 years after its commit in D7. The fix in #8 is not committed, and needs review.
Problem is that we need a flexible width for the 'day name' column. A fixed with in a css file is not the solution.
Please share your solution, review #8 or propose your own patch.
Comment #12
johnvComment #13
johnv