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-69379] Removed obsolete styling in the Add button #422

Merged
merged 2 commits into from Mar 14, 2023
Merged

[JENKINS-69379] Removed obsolete styling in the Add button #422

merged 2 commits into from Mar 14, 2023

Conversation

julieheard
Copy link
Contributor

@julieheard julieheard commented Mar 13, 2023

https://issues.jenkins.io/browse/JENKINS-69379

I have set this to be a draft pull request as I wanted to discuss if this is the correct approach for this fix.

image

When investigating the black line on the add button issue, I noticed that the other 'Add' buttons on the page didn't include 'jenkins-button' on them.

Example of a non-problematic add button on the same page:
image

I removed the 'jenkins-button' bits from the jelly file and tested, and now all of the buttons are looking consistent. Manual testing done with problem button:
image
image

Do you think this is the right approach? Is there something I am missing about removing the 'jenkins-button' bits from the button?

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jtnord jtnord requested a review from janfaracik March 13, 2023 18:16
@janfaracik
Copy link
Contributor

janfaracik commented Mar 13, 2023

Will take a look - thanks!

Looks good to me. Jenkins will hopefully be updated soon to remove the last of the YUI button JS/stylings, once that's done we can update this plugin to restore the .jenkins-button class.

Could you also remove the line button.credentials-add-menu { padding:0 20px 0 10px; } (but keep the class on the button) as its causing unnecessary padding on that button?

@julieheard
Copy link
Contributor Author

Thank you @janfaracik for your comment. I have removed the padding now, if you are happy, I will change from a draft 🙂

@julieheard julieheard marked this pull request as ready for review March 14, 2023 09:45
@julieheard julieheard requested a review from a team as a code owner March 14, 2023 09:45
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!

@raul-arabaolaza raul-arabaolaza merged commit c23caa9 into jenkinsci:master Mar 14, 2023
14 checks passed
@jtnord jtnord added the bug label Mar 14, 2023
@jtnord jtnord changed the title [JENKINS-69379] Removed 'jenkins-button jenkins-button--tertiary' from select.jelly [JENKINS-69379] Removed obsolete styling in the Add button Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants