[GitHub] [maven-fluido-skin] kwin commented on a diff in pull request #23: [MSKINS-169] Image height not effective

[GitBox <gi...@apache.org>]

kwin commented on code in PR #23:
URL: https://github.com/apache/maven-fluido-skin/pull/23#discussion_r866167000


##########
src/main/resources/META-INF/maven/site-macros.vm:
##########
@@ -110,21 +110,21 @@
 #*    *##set ( $imgAlt = ' alt=""' )
 #*  *##end
 #*  *##if( $border )
-#*    *##set ( $imgBorder = ' border="' + $border + '"' )
+#*    *##set ( $imgBorder = 'border: ' + $border + ';' )
 #*  *##else
 #*    *##set ( $imgBorder = "" )
 #*  *##end
 #*  *##if( $width )
-#*    *##set ( $imgWidth = ' width="' + $width + '"' )
+#*    *##set ( $imgWidth = 'width: ' + $width + ';' )
 #*  *##else
 #*    *##set ( $imgWidth = "" )
 #*  *##end
 #*  *##if( $height )
-#*    *##set ( $imgHeight = ' height="' + $height + '"' )
+#*    *##set ( $imgHeight = 'height: ' + $height + ';' )
 #*  *##else
 #*    *##set ( $imgHeight = "" )
 #*  *##end
-<img class="imageLink"$imgSrc$imgAlt$imgBorder$imgWidth$imgHeight/>##
+<img class="imageLink"$imgSrc$imgAlt style="$imgBorder $imgWidth $imgHeight" />##

Review Comment:
   This will add two superfluous spaces in case both `$imgBorder` and `$imgWidth` are both empty. I would recommend doing the concatenation without spaces and make each variable be either the empty string or end with a space. That way you only have at most one superfluous space at the end.



##########
src/main/resources/META-INF/maven/site-macros.vm:
##########
@@ -110,21 +110,21 @@
 #*    *##set ( $imgAlt = ' alt=""' )
 #*  *##end
 #*  *##if( $border )
-#*    *##set ( $imgBorder = ' border="' + $border + '"' )
+#*    *##set ( $imgBorder = 'border: ' + $border + ';' )

Review Comment:
   This should be rather `border-width` (https://developer.mozilla.org/en-US/docs/Web/CSS/border-width) as the css attribute `border` may also set other styles and may lead to some ambiguity and `border-width` is the equivalent of the `border` html attribute. The spec is not really clear in this regard though:  https://maven.apache.org/doxia/doxia-sitetools/doxia-decoration-model/decoration.html#class_bannerLeft



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org