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-73114] avoid conflicts with css classes from bootstrap #9254

Merged
merged 2 commits into from
May 18, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented May 10, 2024

jenkins and bootstrap both have definitions for alert and alert-warning, alert-info and alert-danger. Bootstrap css definitions are included when e.g. the warning-ng plugin is installed and a job is configured to scan for warnings. The css classes from bootstrap are then chosen instead of the ones for Jenkins for things like the admin monitors. To make things unambiguous, add additional classes prefixed with jenkins- and make use of them in code. Keep the old definitions for backward compatibility with plugins.
Follow-up changes are needed in plugins (Mostly those that define admin monitors) and the design-library

After:
for a plugin message:
image

for a core message:
image

See JENKINS-73114.

Originally reported as jenkinsci/customizable-header-plugin#99

Testing done

Manual testing

  1. Create new Jenkins instance, it should have some executors for built-in, this will trigger the corresponding monitor
  2. Visit page /manage/ -> monitors are displayed properly
  3. install warnings-ng plugin
  4. create freestyle job and configure post build action to scan for e.g. stylellint.
  5. run the job once
  6. In the header click on the warning shield -> monitor is displayed properly (If there are monitors from plugins they might not be properly styled here)

Proposed changelog entries

  • Add new CSS classes to avoid conflicts with CSS classes from bootstrap.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@mawinter69 mawinter69 changed the title [JENKINS-73114] avoid conflict of css classes with bootstrap [JENKINS-73114] avoid conflicts of css classes with bootstrap May 10, 2024
@mawinter69 mawinter69 changed the title [JENKINS-73114] avoid conflicts of css classes with bootstrap [JENKINS-73114] avoid conflicts with css classes from bootstrap May 10, 2024
jenkins and bootstrap both have definitions for alert and alert-warning,
alert-info, alert-danger. Bootstrap css definitions are included when
e.g. the warning-ng plugin is installed and a job is configured to scan
for warnings. The css classes from bootstrap are then chosen instead of
the ones for Jenkins for things like the admin monitors.
To make things umambiguous, add additional classes prefixed with
`jenkins-` and make use of them in code. Keep the old definitions for
backward compatibility with plugins.
Followup changes are needed in plugins (Mostly those that define admin
monitors) and the design-library
@NotMyFault NotMyFault added the bug For changelog: Minor bug. Will be listed after features label May 12, 2024
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!


/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
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 May 12, 2024
@timja
Copy link
Member

timja commented May 18, 2024

Can you create a PR for the design library to adapt to this please?

@mawinter69
Copy link
Contributor Author

Can you create a PR for the design library to adapt to this please?

done

@NotMyFault NotMyFault merged commit 90a6ac5 into jenkinsci:master May 18, 2024
17 checks passed
@MarkEWaite MarkEWaite added the developer Changes which impact plugin developers label May 19, 2024
@MarkEWaite
Copy link
Contributor

@mawinter69 can you also identify the plugins that need to be updated to use the new CSS classes? I think it is better to complete the migration from the old CSS classes to the new CSS classes in the most popular plugins.

@mawinter69
Copy link
Contributor Author

Should this maybe downported to LTS 2.452.2? Otherwise it will take quite a while until it arrives in an LTS. Plugins would otherwise need to use weekly 2.459. Specifying both the old and new classes doesn't help when bootstrap loaded so it can't really be made forward compatible

@mawinter69
Copy link
Contributor Author

mawinter69 commented May 20, 2024

I Identified following plugins making use of the any of the alert-* classes. That doesn't necessarily mean they need to be adjusted, e.g. pipeline-stage-view seems to have a bootstrap3 included and relies on the definitions there.

active-directory
alauda-devopy-synvc
artifact-manager-s3
atlassian-bitbucket-server-integration
audit-trail
azure-ad
azure-vm-agents
blue-ocean
branch-api
cloudbees-jenkins-advisor
collabnet
configuration-as-code
copyartifact
customizable-header
dependency-track
dimensionsscm
ec2
github
gitlab-branch-source
gradle
jenkinslint
localization-support
matrix-auth
miniorange-saml-sp
miniorange-two-factor
opentelemetry
performance-signature-dynatrace
pipeline-input-step
pipeline-maven
pipeline-stage-view
pull-request-monitoring
role-strategy
spotinst
ssh-agents
sysdig-secure
sysdig-secure
xcode

@MarkEWaite
Copy link
Contributor

Thanks @mawinter69 ! Is it valid CSS to refer to a CSS class that does not exist? For example, the branch-api plugin today has a line:

<div class="alert alert-warning" role="alert">

Could that be extended to:

<div class="alert jenkins-alert alert-warning" role="alert">

and still be usable on Jenkins versions that don't yet include the new CSS classes?

@mawinter69
Copy link
Contributor Author

mawinter69 commented May 20, 2024

Thanks @mawinter69 ! Is it valid CSS to refer to a CSS class that does not exist?

That would be valid (Jenkins is using all over the place classes without a corresponding css definition to be able to select things in queries). Though only by removing alert and alert-warning there is no interference with bootstrap. And for not breaking plugins that don't have the new classes, the old definitions are still there.

<div class="alert alert-warning" role="alert">

that should become <div class="alert jenkins-alert alert-warning jenkins-alert-warning" role="alert">

krisstern pushed a commit to krisstern/jenkins that referenced this pull request May 24, 2024
…insci#9254)

* [JENKINS-73114] avoid conflict of css classes with bootstrap

jenkins and bootstrap both have definitions for alert and alert-warning,
alert-info, alert-danger. Bootstrap css definitions are included when
e.g. the warning-ng plugin is installed and a job is configured to scan
for warnings. The css classes from bootstrap are then chosen instead of
the ones for Jenkins for things like the admin monitors.
To make things umambiguous, add additional classes prefixed with
`jenkins-` and make use of them in code. Keep the old definitions for
backward compatibility with plugins.
Followup changes are needed in plugins (Mostly those that define admin
monitors) and the design-library

* fix test

(cherry picked from commit 90a6ac5)
basil added a commit to basil/jenkins that referenced this pull request Jun 21, 2024
MarkEWaite pushed a commit that referenced this pull request Jun 21, 2024
…ap" on `stable-2.452` (#9409)

Revert "[JENKINS-73114] avoid conflicts with css classes from bootstrap (#9254)"

This reverts commit b87adb7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants