jenkinsci / badge-plugin

Jenkins Badge plugin
https://plugins.jenkins.io/badge/
MIT License
32 stars 43 forks source link

Use size class for all icons, not just symbols #149

Closed MarkEWaite closed 5 months ago

MarkEWaite commented 6 months ago

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 https://github.com/jenkinsci/badge-plugin/issues/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
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue
mPokornyETM commented 6 months ago

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

MarkEWaite commented 6 months ago

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 commented 6 months ago

Maybe your proposed changes are not yet submitted?

sorry, I mean my comments

MarkEWaite commented 6 months ago

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.

strangelookingnerd commented 6 months ago

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.