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.

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

Merged
merged 30 commits into from
May 11, 2024

Conversation

cronik
Copy link
Contributor

@cronik cronik commented May 2, 2024

Allow badges to use Jenkins symbols and color references. Update info/warn/error badges to use symbols by default.

Screenshot 2024-05-02 at 11 13 25 AM

Testing done

Used run-fast to create badges used in the screenshots.

Submitter checklist

Preview Give feedback

Allow badges to use Jenkins symbols and color references. Update info/warn/error badges to use symbols by default.
@cronik cronik requested a review from a team as a code owner May 2, 2024 17:09
Copy link
Contributor

@MarkEWaite MarkEWaite left a 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

@cronik
Copy link
Contributor Author

cronik commented May 3, 2024

Thanks for those enhancements.

The reason for adding color as a constructor instead of a setter was down to the fact that all the sub classes of AddBadgeStep don't support the color parameter, but the docs generator would include it. So in the interest of keeping the changes to a min I went the constructor route. Another option might be to adjust the Step type hierarchy a bit to get everything to align properly. I'll test some stuff out, maybe it's not as much additional change as I had originally thought.

Refactor type hierarchy so that the optional color parameter only
applies to the generic addBadge step. Reverted previous changes
to the docs generator.
@cronik
Copy link
Contributor Author

cronik commented May 3, 2024

@MarkEWaite I managed to rework the PR to move the color parameter to a setter and only apply it to the generic addBadge step. Was able to revert the doc generator changes as result.

@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 4, 2024

Thanks @cronik! I saw that there was no online help for the addBadge parameters, so I added online help for them and for a few other items.

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.

@MarkEWaite MarkEWaite self-requested a review May 4, 2024 02:33
Copy link
Contributor

@MarkEWaite MarkEWaite left a 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
Copy link
Contributor

@MarkEWaite MarkEWaite May 4, 2024

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.

Suggested change
* Copyright (c) 2004-2010, Sun Microsystems, Inc., Serban Iordache
* Copyright (c) 2024, Kyle Cronin

MarkEWaite added 5 commits May 4, 2024 05:59
Jenkins provided icons described first, then a description, more
detail, and examples are provided for icons provided by the plugin.
@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 4, 2024

The online help that I added is working as expected on the addBadge step for all parameters except the color parameter. I don't understand why the color parameter behaves differently in the online help than the help for the icon, id, link, and text parameters. Unfortunately, I've run out of ideas to explore. I've left the source file for the text help in the pull request, even though it is not displayed to the user from the "Pipeline syntax" page.

I think that it is an improvement to have online help, even if the help is not displayed for the color parameter. The color parameter is well documented in the README.

The online help is visible from the "/pipeline-syntax" page when the addBaadge entry is selected in the dropdown menu and the "?" icon is clicked that is next to each parameter. That text will be extracted into the "Pipeline steps reference" page after this pull request is merged and released.

In case you'd like to see how it looks, here is a screenshot of the addBadge online help from the Pipeline syntax snippet generator:

screencapture-rhel-8-a-markwaite-net-8080-jenkins-job-Pipeline-pipeline-syntax-2024-05-04-06_46_31-edit

@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 4, 2024

While testing the combined pull request:

I discovered that var(--yellow) does not change the icon color. There is an example in the README that says:

addBadge(icon: 'symbol-star', text: 'A star', color: 'var(--yellow)')

Can you help me identify the mistake I am making when using that example?

I see a similar surprise with the example:

addBadge(icon: 'symbol-star', text: 'A star', color: 'rgb(239, 245, 66)')

I would be just fine removing those examples if they are not expected to change the color of the symbol.

@MarkEWaite
Copy link
Contributor

The createSummary behavior has changed from the 1.9.1 release to this pull request. The word "span" is incorrectly inserted into the message when I use appendText as suggested in one of the examples.

Before this change

When I use the following Pipeline:

def summary = createSummary(icon: 'green')
summary.appendText('Completed text added.', true)
summary.appendText(' Bold text added.', false, true, false, 'black')

before

After this change

after

@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 5, 2024

The createSummary behavior has changed from the 1.9.1 release to this pull request. The word "span" is incorrectly inserted into the message when I use appendText as suggested in one of the examples.

Fixed in 3b4c000

Copy link
Contributor

@mPokornyETM mPokornyETM left a comment

Choose a reason for hiding this comment

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

LGTM

@cronik
Copy link
Contributor Author

cronik commented May 6, 2024

@MarkEWaite Thanks for the collaboration on this PR!

I discovered that var(--yellow) does not change the icon color. There is an example in the README that says:

One issue is that the color style is not set on l:icon. But after some additional testing and digging I realized that even if <l:icon style="${badgeStyle}" is set, l:icon does not respect it when the icon path starts with symbol-. So I think some revised docs are in order to indicate that only Jenkins palette and semantic color names and classes are support for symbol icon references. For other icon references those css references should work as expected with the update I just pushed.

It should be noted that var(--yellow) works with addShortText. I updated the help and docs to make this more clear.

@cronik
Copy link
Contributor Author

cronik commented May 6, 2024

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.

@MarkEWaite I'm not entirely sure what it means to adopt a plugin, but I suppose I could.

@MarkEWaite
Copy link
Contributor

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

@MarkEWaite
Copy link
Contributor

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

@strangelookingnerd
Copy link
Contributor

@MarkEWaite I took the freedom of adding my remark from #130 (comment) to this PR.
This is by not appending plugin-ionicons-api to the icon name (in case they start with symbol-) and rather expect the icon name to follow the pattern of symbol-icon-name plugin-plugin-name as noted in https://weekly.ci.jenkins.io/design-library/Symbols.
I also updated the docs and pipeline syntax help. Let me know what you think and I'll gladly merge and release this PR.

@strangelookingnerd strangelookingnerd force-pushed the feature/jenkins-symbols branch from 6462329 to 1c48304 Compare May 10, 2024 09:06
@MarkEWaite
Copy link
Contributor

@MarkEWaite I took the freedom of adding my remark from #130 (comment) to this PR. This is by not appending plugin-ionicons-api to the icon name (in case they start with symbol-) and rather expect the icon name to follow the pattern of symbol-icon-name plugin-plugin-name as noted in https://weekly.ci.jenkins.io/design-library/Symbols. I also updated the docs and pipeline syntax help. Let me know what you think and I'll gladly merge and release this PR.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not addShortText() with an id to removeBadges() later, but there is a workaround (to document?)
4 participants