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

Use 'jenkins-button' for tables #9131

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Apr 6, 2024

Small one to replace .jenkins-table__button with just .jenkins-button. The two classes are super similar and we're not getting any advantages from having them separated.

Couple of visual changes:

  • Buttons with a colour applied to them now reflect the colour in the buttons background

Before
image

After
image

  • Buttons are slightly bubblier/rounded

Testing done

  • All buttons in core have had their classes changed and look as expected

Proposed changelog entries

  • N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

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.

LGTM, I checked the changes here and I looked for any plugins that looked funny and didn't find any but the class hasn't been removed so I wouldn't expect any

@timja timja added web-ui The PR includes WebUI changes which may need special expertise skip-changelog Should not be shown in the changelog labels Apr 7, 2024
@timja timja requested a review from a team April 7, 2024 07:20
@timja
Copy link
Member

timja commented Apr 7, 2024

/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 Apr 7, 2024
@daniel-beck daniel-beck removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 8, 2024
@timja timja requested a review from daniel-beck April 8, 2024 12:55
@NotMyFault NotMyFault requested a review from daniel-beck April 8, 2024 21:42
@daniel-beck daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Apr 9, 2024
@timja
Copy link
Member

timja commented Apr 9, 2024

/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 Apr 9, 2024
@NotMyFault NotMyFault merged commit 16a6575 into jenkinsci:master Apr 10, 2024
17 checks passed
@timja timja deleted the use-jenkins-button-for-tables branch April 10, 2024 18:20
@mawinter69
Copy link
Contributor

mawinter69 commented Jul 13, 2024

This causes a regression. The buttons in tables are then no longer resized when the icon size is changed.
The background of the button is also flowing over the row border
Also there is now hardly any difference in icon size between large and medium table size
Small icons:
image

Medium icons
image

Large icons:
image

@MarkEWaite
Copy link
Contributor

Thanks for the comment @mawinter69 . This will first be visible to LTS users in Jenkins 2.462.1 that is scheduled to release 7 Oct 2024. The impact of the issues does not seem very severe to me, but I agree that it is a change of behavior and it is unexpected that small, medium, and large have very little difference now.

If a fix is provided, should it also be considered for a backport?

If a backport is needed, then an issue needs to be created on https://issues.jenkins.io so that it can be marked as an LTS candidate.

@mawinter69
Copy link
Contributor

mawinter69 added a commit to mawinter69/jenkins that referenced this pull request Jul 18, 2024
With jenkinsci#9131 the jenkins-table__button was replaced at certain places with
jenkins-button. But this change caused two regressions:
- The icon size for large table was almost the same as that for medium
  tables
- The icons wrapped in jenkins-button were not resizable
- with small table the buttons overflowed the table cell

This change restores the old icon size for large tables and adds
additional css rules for jenkins-button inside tables so they are
properly resized and the padding is correct
mawinter69 added a commit to mawinter69/jenkins that referenced this pull request Jul 18, 2024
With jenkinsci#9131 the jenkins-table__button was replaced at certain places with
jenkins-button. But this change caused two regressions:
- The icon size for large table was almost the same as that for medium
  tables
- The icons wrapped in jenkins-button were not resizable
- with small table the buttons overflowed the table cell

This change restores the old icon size for large tables and adds
additional css rules for jenkins-button inside tables so they are
properly resized and the padding is correct
NotMyFault pushed a commit that referenced this pull request Jul 21, 2024
[JENKINS-73543] make icon in buttons in tables resizable

With #9131 the jenkins-table__button was replaced at certain places with
jenkins-button. But this change caused two regressions:
- The icon size for large table was almost the same as that for medium
  tables
- The icons wrapped in jenkins-button were not resizable
- with small table the buttons overflowed the table cell

This change restores the old icon size for large tables and adds
additional css rules for jenkins-button inside tables so they are
properly resized and the padding is correct
NotMyFault pushed a commit to NotMyFault/jenkins that referenced this pull request Aug 14, 2024
[JENKINS-73543] make icon in buttons in tables resizable

With jenkinsci#9131 the jenkins-table__button was replaced at certain places with
jenkins-button. But this change caused two regressions:
- The icon size for large table was almost the same as that for medium
  tables
- The icons wrapped in jenkins-button were not resizable
- with small table the buttons overflowed the table cell

This change restores the old icon size for large tables and adds
additional css rules for jenkins-button inside tables so they are
properly resized and the padding is correct

(cherry picked from commit 19d568c)
@mawinter69
Copy link
Contributor

caused JENKINS-73668

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 skip-changelog Should not be shown in the changelog squash-merge-me Unclean or useless commit history, should be merged only with squash-merge web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants