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

[JENKINS-71326] - Support colored badges in breadcrumb dropdown #8853

Conversation

eduardojandre
Copy link
Contributor

See JENKINS-71326.

To solve the problem I had to:
1-Update the backend to expose the badge severity field to the API used by dropdowns.
2-Update the dropdown and side menu to use the badge severity to set the corresponding CSS class.
3-Update the CSS files so that the badge in the dropdown and side menu behaves similarly to the normal menu.

Copy link

welcome bot commented Jan 10, 2024

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

@mawinter69
Copy link
Contributor

Can you provide a screenshot please how it looks.
Ideally it looks the same as the badges on the manage page.
image

@mawinter69
Copy link
Contributor

Replaces #8798

@eduardojandre
Copy link
Contributor Author

Sure.
sidemenu
Dropdown

@mawinter69
Copy link
Contributor

mawinter69 commented Jan 10, 2024

The badges on the manage page have a slightly different styling. They have a small color gradient and an animation
image
image
image
image
See

for the styling

Just an idea, we have now 3 files (5 calls) where we render a badge. Maybe we can move that into an own jelly tag e.g. core/src/main/resources/lib/layout/badge.jelly so we just call it via <l:badge badge="${attrs.badge"/>.
This gets its own styling (under war/src/main/scss/components/_badge.scss) which ensures that wherever it is used it looks the same. And when we adjust we have just a single place where we need to do changes. This also allows plugins to easily reuse the badge jelly and directly get it properly styled.

@eduardojandre
Copy link
Contributor Author

Yes, I did not notice the gradient on my tests, the animation I didn't replicate because I was not sure if this was intentional or not.

But this was something that bothered me a little, having the same component replicated in multiple files, although it was previous behavior. It seemed like a bigger change and I didn't know the best way to do it.

I will try your suggestion.

Thanks.

@eduardojandre
Copy link
Contributor Author

I pushed another commit with changes, can you please take a look?

Now there is a jelly that is used on Manage screen and on the side menu, but the dropdown is loaded directly on javascript through ajax so I was not able to use the same jelly view on that one. (but I used the same CSS class on it)

war/src/main/scss/components/_badges.scss Outdated Show resolved Hide resolved
war/src/main/scss/components/_dropdowns.scss Outdated Show resolved Hide resolved
war/src/main/scss/components/_section.scss Outdated Show resolved Hide resolved
war/src/main/scss/components/_side-panel-tasks.scss Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/task.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/badge.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/badge.jelly Outdated Show resolved Hide resolved
war/src/main/js/components/dropdowns/templates.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mawinter69 mawinter69 left a comment

Choose a reason for hiding this comment

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

tested locally and it looks good

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

You forgot to run cd war/ ; yarn run lint:fix.

core/src/main/resources/lib/layout/badge.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/badge.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/badge.jelly Outdated Show resolved Hide resolved
@NotMyFault NotMyFault added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Jan 11, 2024
@NotMyFault NotMyFault requested review from a team January 11, 2024 11:43
@eduardojandre
Copy link
Contributor Author

You forgot to run cd war/ ; yarn run lint:fix.

Sorry, just fixed.

I applied the suggestion of "@SInCE TODO", when the TODO will be replace by the version? Is it in the pipeline?

@NotMyFault
Copy link
Member

You forgot to run cd war/ ; yarn run lint:fix.

Sorry, just fixed.

I applied the suggestion of "@SInCE TODO", when the TODO will be replace by the version? Is it in the pipeline?

That happens when a maintainer runs the https://github.com/jenkinsci/jenkins/blob/master/update-since-todo.py

@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jan 12, 2024
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

Looks fine security wise

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks, looks nice :)

@NotMyFault NotMyFault requested a review from a team January 13, 2024 15:05
@NotMyFault NotMyFault requested a review from timja February 9, 2024 08:07
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 9, 2024
@NotMyFault NotMyFault merged commit 8b94924 into jenkinsci:master Feb 12, 2024
17 checks passed
Copy link

welcome bot commented Feb 12, 2024

Congratulations on getting your very first Jenkins core pull request merged 🎉🥳

This is a fantastic achievement, and we're thrilled to have you as part of our community! Thank you for your valuable input, and we look forward to seeing more of your contributions in the future!

We would like to invite you to join the community chats and forums to meet other Jenkins contributors 😊
Don't forget to check out the participation page to learn more about how to contribute to Jenkins.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants