Use size class for all icons, not just symbols

[MarkEWaite]

Use size class for all icons, not just symbols

Symbols do not require height or width but icons without height and width will render at their original size instead of rendering at the standard size for icons.

Fixes #148

Testing done

Confirmed that before this change, the following icon renders incorrectly as a 48x48 image:

addBadge(icon: '/images/48x48/light-grey.gif', text: 'large icon')

Confirmed that before this change, the following icon renders correctly:

addBadge(icon: 'symbol-star plugin-ionicons-api', text: 'a starred build')

Confirmed that after this change, the following icon renders correctly as a 16x16 image:

addBadge(icon: '/images/48x48/light-grey.gif', text: 'large icon')

Confirmed that after this change, the following icon renders correctly:

addBadge(icon: 'symbol-star plugin-ionicons-api', text: 'a starred build')

Submitter checklist

Preview Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[mPokornyETM]

my proposal are not mandatory, @MarkEWaite let me know if you want to change it or not.

[MarkEWaite]

my proposal are not mandatory, @MarkEWaite let me know if you want to change it or not.

Thanks for your comment @mPokornyETM . I didn't see any proposed changes in your comment. Maybe your proposed changes are not yet submitted?

[mPokornyETM]

Maybe your proposed changes are not yet submitted?

sorry, I mean my comments

[MarkEWaite]

I've replaced indexOf with startsWith so that the code is clearer. Interactive testing of the failure case shows that the change is behaving as I had expected.

I'd like a review from @strangelookingnerd if time allows. However, I'd also like to resolve the bug before the end of the week if possible. My interactive testing of #130 did not detect that regression and I'd like to fix it promptly so that other users are not disturbed by it.

[mPokornyETM]

LGTM

[mPokornyETM]

I am not taotally happy with hardcoded numbers. Are there no spacing in the design-library ?

[MarkEWaite]

That's a very good question. @janfaracik is there a better way to express this? I'm a novice with Jelly and even more of a novice with CSS and page layout.

[strangelookingnerd]

@MarkEWaite I think we could get away with removing the width and height attributes and delegate the sizing to class here.
By removing the if check in

badge-plugin/src/main/java/com/jenkinsci/plugins/badge/action/BadgeAction.java

Line 202 in 241923d

if (isJenkinsSymbolRef(this.iconPath)) {

the icon would always use the class icon-sm and thus be sized correctly. No hardcoded size required.

[MarkEWaite]

Thanks very much for the recommendation. That makes the code much simpler and avoids conditional logic in the jelly file. Implemented in 535e18f

[strangelookingnerd]

I've replaced indexOf with startsWith so that the code is clearer. Interactive testing of the failure case shows that the change is behaving as I had expected.

I'd like a review from @strangelookingnerd if time allows. However, I'd also like to resolve the bug before the end of the week if possible. My interactive testing of #130 did not detect that regression and I'd like to fix it promptly so that other users are not disturbed by it.

I‘ll have a look at it later today, tomorrow latest.