Narrow the first holdings tab column to 25% width #3096
Conversation
| @@ -217,6 +217,12 @@ | |||
| .tab-pane .result { | |||
| margin-left: 0; | |||
| } | |||
| .holdings-tab table { | |||
There was a problem hiding this comment.
@crhallberg I know you suggested a class name on the table itself, but I think this does the trick? If there are counter-examples of tables on the holdings tab that should be treated differently, then I can narrow it down.
There was a problem hiding this comment.
I have no counter examples, just thought it would be better for customization.
There was a problem hiding this comment.
Actually, I think we do need to use a more specific class, because otherwise the "Most Recent Received Issues" table will get crushed: https://github.com/vufind-org/vufind/blob/dev/themes/bootstrap3/templates/RecordTab/holdingsils.phtml#L154
Now whether it makes any sense for this to be a single-column table instead of some other HTML structure is a separate question. But it is at present, so we should avoid breaking it.
There was a problem hiding this comment.
I just took a look at this, and I wonder if we'd be better off setting min-width/max-width instead of a fixed 25% width, to give it just a little more "wiggle room." When I tested with the Demo driver, I ended up with this scenario:
Before (dev branch):
After (this PR):
I think that looks worse after the change, though I realize that the copy data provided by the Demo driver is not very realistic, so this exact scenario is pretty unlikely in real life. Still, it feels like setting a min and a max might accommodate a broader range of scenarios without interfering with your primary intent of making the left column take up less than half the screen when it doesn't need all that space.
Also, have you tested this on mobile? I wonder if 25% is too narrow on a very tiny display, and maybe a pixel max would be better than a percentage max.
Obviously, I defer to whatever @crhallberg thinks... but just trying to get the ball rolling. :-)
|
The reason we need a fixed width (for now) is we want things to line up from table to table. If we give them flexibility, they won't line up if your example sits next to one whose label is just "Online" and doesn't expand. Is this a huge issue? No. But that's the consideration. |
|
@crhallberg, do you think it's worth mocking up an example of your counter-argument, so we can decide which approach looks better? I think I might prefer inconsistent table sizes across holdings locations over unnecessarily wrapped/crushed-looking text. But maybe I wouldn't if I actually saw it. |
|
Behold! The horror!
In all honesty, I thought this would be worse. And it might be if there are multiple campuses with one location each. I also had to set |
| table-layout: auto; | ||
| th:first-child, td:first-child { | ||
| width: 25%; | ||
| } |
There was a problem hiding this comment.
In response to the discussion on the main page, I think we can remove this part and let the table auto layout. We can do a min-width if we're worried about collapse of like 20%.
There was a problem hiding this comment.
@maccabeelevine, what do you think? Can you resolve conflicts here and adjust width as discussed (unless you prefer a different approach)?
I agree that auto-width surprisingly seems to work fine, as long as it's limited to that particular table. I don't think we need a min-width (but can add that if folks disagree). I think this is good now. |
There was a problem hiding this comment.
Thanks, @maccabeelevine, looks good to me! I'll wait to merge this until @crhallberg also gives it a passing review.
|
Do we want to put a minimum width on these columns? You can't use a percent but I was able to approximate a percentage by using |
…beelevine/vufind into holdings-tab-column-widths
@crhallberg Yes I think that's a good idea to prevent the worst effects of making it dynamic. I made the change to both th and td cells. |
|
Thanks, @crhallberg and @maccabeelevine. |
|
In retrospect, I should have changed the commit message when merging this since it has evolved a bit since it started. Alas, didn't think of that soon enough. |
That's my bad. But in some random chaotic version of the data it will be exactly 25% width! :) |
Based on discussion with @crhallberg for #3094. Tables in the holdings tab are only 50/50 as a default from bootstrap; there was likely no specific decision for these tables. The first column tends to be pretty short, while the second may be more variable. In #3094 we are adding a whole lot of content to the second column that would benefit from some extra space. 25/75 may be reasonable.
Auto-layout without explicit width percentages would solve the same problem but make each table look different and scattered. I prefer the fixed layout and I think @crhallberg agreed in discussion earlier today.