feat(infobox): cleanup header image display #4197
Conversation
|
can you check if it breaks infoboxes with fixed image size? example page: https://liquipedia.net/starcraft2/Stimpack from preview it looks broken to me but i can not check the css changes applied with it on mobile with this pr: |
Yeah the CSS centers it correctly.
|
|
As I mentioned in the comment, I can't do that because of differences in how lightmode/darkmode image is swapped in the infobox. It uses its own classes that aren't applied to the |
| size = size or '' | ||
| if tonumber(size) then | ||
| size = tonumber(size) .. 'px' | ||
| local infoboxImage = mw.html.create('div'):addClass('infobox-image'):addClass(mode) |
There was a problem hiding this comment.
Could the css be tweaked so infobox-image is only needed to be set on the wrapper?
There was a problem hiding this comment.
The way the infobox itself is all written, it's got classless div rows, and then each of those rows is filled with different things (headers/images/rows/etc.). It wouldn't make much sense to only change one of these and have the rest be as before, imo.
There was a problem hiding this comment.
<div class="...-infobox">
...
<div>
<div class="infobox-image">
<div class="lightmode">
...
</div>
</div>
</div>
...
</div>
How about this
There was a problem hiding this comment.
Would still require tweaking some CSS, but wouldn't be too hard.
Idk if there's any conventions on "adjective" class names needing to be applied to an element with an existing "noun" class name (if that makes sense?) Obviously, on a technical level it makes absolutely no difference, just wondering how people feel about it in general (i.e. "lightmode" what? vs "lightmode" "infobox image").
There was a problem hiding this comment.
do we even need the inner div (which sets the lightmode)?
if using module:Image it would get set in the image itself
<div class="...-infobox">
...
<div>
<div class="infobox-image [infobox-fixed-size-image]">
[File:...|class=show-when-light-mode][File:...|class=show-when-dark-mode] <-- Module Image call
</div>
</div>
...
</div>would be in line with how other infobox rows are displayed too
(obviously in this case it would merke the 2 images into 1 since they are the same, just for showcasing with 2 different light/dark mode)
tested on sc2 with
- case with light + dark mode
- case with only light mode (hence using it for darkmode also
- case with fixed size (stimpack)
no extra css changes needed (only the one already suggested in the PR)
can probably even kick some css
| if tonumber(size) then | ||
| size = tonumber(size) .. 'px' | ||
| local infoboxImage = mw.html.create('div'):addClass('infobox-image'):addClass(mode) | ||
| if Logic.isNumeric(size) then | ||
| infoboxImage:addClass('infobox-fixed-size-image') |
Co-authored-by: Rikard Blixt <[email protected]>
Summary
Cleaned up the output for infobox header images a bit so that it doesn't end up producing two of the same image when there is no need. Also removed some unnecessary
divwrappers by moving their functionality into existing css classes.darkmode/lightmodeallmodeHow did you test this change?
Tested on dev.