Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(infobox): cleanup header image display #4197

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(infobox): cleanup header image display #4197

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15146fd
feat(infobox): cleanup header image display
iMarbot Apr 20, 2024
a06f4af
use stringutils instead
iMarbot Apr 20, 2024
282d449
Rework to use Image module
iMarbot Apr 21, 2024
aa7dadc
Limit image thumbnail vertical height too
iMarbot Apr 21, 2024
729c22f
Update components/infobox/commons/infobox_widget_header.lua
iMarbot Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 23 additions & 29 deletions components/infobox/commons/infobox_widget_header.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
--

local Class = require('Module:Class')
local Image = require('Module:Image')
local Logic = require('Module:Logic')
local Lua = require('Module:Lua')
local String = require('Module:StringUtils')

local Widget = Lua.import('Module:Infobox/Widget')

Expand Down Expand Up @@ -92,49 +95,40 @@ end
---@param size number|string|nil
---@return Html?
function Header:_image(fileName, fileNameDark, default, defaultDark, size)
if (fileName == nil or fileName == '') and (default == nil or default == '') then
local imageName = fileName or default
local imageDarkname = fileNameDark or fileName or defaultDark or default

if String.isEmpty(imageName) and String.isEmpty(imageDarkname) then
return nil
end

local imageName = fileName or default
---@cast imageName -nil
local infoboxImage = Header:_makeSizedImage(imageName, fileName, size, 'lightmode')
if imageName == imageDarkname then
return mw.html.create('div'):node(Header:_makeSizedImage(imageName, size))
end
iMarbot marked this conversation as resolved.
Show resolved Hide resolved

imageName = fileNameDark or fileName or defaultDark or default
---@cast imageName -nil
local infoboxImageDark = Header:_makeSizedImage(imageName, fileNameDark or fileName, size, 'darkmode')
local infoboxImage = Header:_makeSizedImage(imageName, size, 'lightmode')
local infoboxImageDark = Header:_makeSizedImage(imageDarkname, size, 'darkmode')

return mw.html.create('div'):node(infoboxImage):node(infoboxImageDark)
end

---@param imageName string
---@param fileName string?
---@param imageName string?
---@param size number|string|nil
---@param mode string
---@return Html
function Header:_makeSizedImage(imageName, fileName, size, mode)
local infoboxImage = mw.html.create('div'):addClass('infobox-image ' .. mode)
---@param mode string?
---@return Html?
function Header:_makeSizedImage(imageName, size, mode)
if String.isEmpty(imageName) then
return
end

-- Number (interpret as pixels)
size = size or ''
if tonumber(size) then
size = tonumber(size) .. 'px'
local infoboxImage = mw.html.create('div'):addClass('infobox-image'):addClass(mode)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the css be tweaked so infobox-image is only needed to be set on the wrapper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Collaborator

@Rathoz Rathoz Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<div class="...-infobox">
 ...
 <div>
  <div class="infobox-image">
   <div class="lightmode">
     ...
   </div>
  </div>
 </div>
 ...
</div>

How about this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Copy link
Collaborator

@hjpalpha hjpalpha Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-04-22 074735
(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)

https://liquipedia.net/commons/index.php?title=Module%3AInfobox%2FWidget%2FHeader%2Fdev&type=revision&diff=703437&oldid=703225

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 Logic.isNumeric(size) then
infoboxImage:addClass('infobox-fixed-size-image')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this

-- Percentage (interpret as scaling)
elseif size:find('%%') then
local scale = size:gsub('%%', '')
local scaleNumber = tonumber(scale)
if scaleNumber then
size = 'frameless|upright=' .. (scaleNumber / 100)
infoboxImage:addClass('infobox-fixed-size-image')
end
-- Default
else
size = '600px'
size = '600x1000px'
end

local fullFileName = '[[File:' .. imageName .. '|center|' .. size .. ']]'
infoboxImage:wikitext(fullFileName)
infoboxImage:wikitext(Image.display(imageName, nil, {size = size}))

return infoboxImage
end
Expand Down
1 change: 1 addition & 0 deletions stylesheets/commons/Infobox.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ Author(s): FO-nTTaX
}

.fo-nttax-infobox > div > div.infobox-image {
text-align: center;
padding: 0;
width: 100%;
}
Expand Down
Loading