-
Notifications
You must be signed in to change notification settings - Fork 41
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.
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
Support design library symbols and colors #130
Support design library symbols and colors #130
Conversation
Allow badges to use Jenkins symbols and color references. Update info/warn/error badges to use symbols by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much @cronik. I like the change!
I've made some documentation updates while I was testing the pull request. If you object to those changes, you are welcome to remove them or revert them.
The color
parameter that has been added to the command is not optional even though it is documented as optional in the javadoc and needs to be optional for compatibility with existing installations.
I haven't done the research to understand why that parameter is mandatory, though I suspect it is due to the new constructor that was added to the BadgeAction class. I believe that new parameters are usually added with DataBoundSetter rather than by adding a new constructor
Thanks for those enhancements. The reason for adding |
Refactor type hierarchy so that the optional color parameter only applies to the generic addBadge step. Reverted previous changes to the docs generator.
@MarkEWaite I managed to rework the PR to move the color parameter to a setter and only apply it to the generic |
Better describe the bug symbol example Add online help for addBadge parameters
Thanks @cronik! I saw that there was no online help for the Since the plugin is up for adoption, this pull request would be a great one for you to use to adopt the plugin. Are you interested in adopting the plugin? I'm happy to continue as a reviewer and tester, since I use the badge plugins and I maintain the embeddable build status plugin. If you are willing to adopt the plugin, I think it would be good for us to take the step to switch to automated plugin release so that the plugin is released each time a new enhancement is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes have passed all my interactive tests (oops - tests were incomplete, see later comments)
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2004-2010, Sun Microsystems, Inc., Serban Iordache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that you want to assign copyright for this file to Serban Iordache. I think that you can either remove the explicit copyright statement or add your additional copyright if the original source was derived from something that Serban Iordache created.
* Copyright (c) 2004-2010, Sun Microsystems, Inc., Serban Iordache | |
* Copyright (c) 2024, Kyle Cronin |
Jenkins provided icons described first, then a description, more detail, and examples are provided for icons provided by the plugin.
The online help that I added is working as expected on the I think that it is an improvement to have online help, even if the help is not displayed for the The online help is visible from the "/pipeline-syntax" page when the In case you'd like to see how it looks, here is a screenshot of the |
While testing the combined pull request: I discovered that
Can you help me identify the mistake I am making when using that example? I see a similar surprise with the example:
I would be just fine removing those examples if they are not expected to change the color of the symbol. |
The Before this changeWhen I use the following Pipeline:
After this change |
src/main/java/com/jenkinsci/plugins/badge/action/AbstractAction.java
Outdated
Show resolved
Hide resolved
Fixed in 3b4c000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@MarkEWaite Thanks for the collaboration on this PR!
One issue is that the color style is not set on It should be noted that |
This addition is not needed after refactoring type hierarchy.
@MarkEWaite I'm not entirely sure what it means to adopt a plugin, but I suppose I could. |
Adopting a plugin means that you're willing to become a maintainer of the plugin. Plugin maintainers review pull requests, review issue reports, merge pull requests they have reviewed, and release new versions. The frequently asked questions should address many questions. @strangelookingnerd just submitted the email message and pull request to become a maintainer. I suspect that @strangelookingnerd would be happy to have an additional maintainer to help with code reviews and other maintenance work. The plugin needs more modernization as described in the "Improve a plugin" tutorial. It also needs to have automated release enabled so that bug fixes and enhancement requests are automatically released when they are merged. |
@strangelookingnerd thanks very much for adopting the plugin. I'm willing to merge and release it if needed, but would prefer that you would do that as a new maintainer. I'm happy to answer questions or provide other encouragement as needed. |
@MarkEWaite I took the freedom of adding my remark from #130 (comment) to this PR. |
6462329
to
1c48304
Compare
That looks great to me. I made minor changes to the documentation. I think it is ready to be squash merged and released. Thanks @cronik for the pull request and thanks @strangelookingnerd for adopting the plugin |
Allow badges to use Jenkins symbols and color references. Update info/warn/error badges to use symbols by default.
Testing done
Used
run-fast
to create badges used in the screenshots.Submitter checklist