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

Update submit buttons to use .jenkins-button classes #7203

Merged
merged 14 commits into from
Oct 29, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Oct 2, 2022

Small PR which updates the submit buttons to use the .jenkins-button classes, along with @NotMyFault's work this should be the majority of buttons converted 🚀

Before

image

After

image

Proposed changelog entries

  • Update submit buttons to use .jenkins-button classes.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

commit 0767df8
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Oct 2 13:11:29 2022 +0100

    Update check.jelly

commit a2db868
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Oct 2 13:04:27 2022 +0100

    Add is primary attr, use destructive color

commit 1e3fd44
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Sep 27 19:46:36 2022 +0100

    Lint + fix apply button

commit 6afac49
Merge: 3d4cdbb 23a79b4
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Sep 27 19:30:27 2022 +0100

    Merge branch 'master' into new-buttons-2-bottom-bar

commit 3d4cdbb
Merge: b5c195f 1afd369
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Aug 16 13:10:54 2022 +0100

    Merge branch 'master' into new-buttons-2-bottom-bar

commit b5c195f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Jul 8 15:24:12 2022 +0100

    Cleanup

commit ebc774f
Merge: efbcaf4 418ab06
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Jul 8 15:15:02 2022 +0100

    Merge branch 'new-buttons-1-dashboard' into new-buttons-2-bottom-bar

commit 418ab06
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jul 7 22:41:32 2022 +0100

    Update buttons-deprecated.less

commit 2798afa
Merge: 296e309 6de288f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jul 7 22:40:35 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit 296e309
Merge: 1e646ff 22dcefc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jul 7 14:50:14 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit 1e646ff
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jul 6 22:59:13 2022 +0100

    Use rem, fix cursor for dashboard icon size

commit 21d4c73
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jul 6 22:56:05 2022 +0100

    Update with master, make minor changes

commit 1d59bd4
Merge: f6fc19b 644a261
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jul 6 22:48:59 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit f6fc19b
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 26 22:58:12 2022 +0100

    Update dashboard.less

commit 665daa0
Merge: a57f13a c60ea92
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 26 22:52:33 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit a57f13a
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Jun 20 19:01:18 2022 +0100

    Update simple-page.less

commit efbcaf4
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 22:32:50 2022 +0100

    Update buttons.less

commit 84e98a2
Merge: ac594f3 2d3695d
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 22:27:40 2022 +0100

    Merge branch 'new-buttons-1-dashboard' into new-buttons-2-bottom-bar

commit 2d3695d
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 22:24:32 2022 +0100

    Update theme.less

commit 361f06c
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 22:22:48 2022 +0100

    Split out vars

commit 205336f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 21:48:33 2022 +0100

    Move files about, fix focus state, fix colors

commit 204bafb
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 21:00:57 2022 +0100

    Update buttons to support colors

commit 1d4be07
Merge: c407577 2391319
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 20:01:19 2022 +0100

    Merge branch 'move-manage-jenkins-less' into new-buttons-1-dashboard

commit c407577
Merge: 0c8d538 65fcda1
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jun 19 20:01:02 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit 2391319
Merge: c9e2823 ac66b47
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:47:45 2022 +0100

    Merge branch 'master' into move-manage-jenkins-less

commit c9e2823
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:35:37 2022 +0100

    Update colors

commit c78dfd6
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:33:47 2022 +0100

    Update theme.less

commit 0996e75
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:32:52 2022 +0100

    Update theme.less

commit e57bc28
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:29:15 2022 +0100

    Update theme.less

commit 69c1633
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 16 17:00:06 2022 +0100

    Update style.less

commit e21d4e7
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 15 19:03:50 2022 +0100

    Update breadcrumbs to use new item mixin

commit 90fc4ba
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 15 18:57:51 2022 +0100

    Rename mixin

commit 28d0638
Merge: d1f42ac 65fcda1
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Jun 14 23:35:10 2022 +0100

    Merge branch 'master' into move-manage-jenkins-less

commit d1f42ac
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Jun 10 11:13:58 2022 +0100

    Update table.less

commit 13d5729
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jun 9 10:03:01 2022 +0100

    Move from px to rem

commit 68f5425
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 8 16:40:41 2022 +0100

    Update theme.less

commit 929e2d5
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 8 16:28:06 2022 +0100

    Update mixins.less

commit 8c4b466
Merge: 8b22270 77a36fc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 8 16:27:25 2022 +0100

    Merge branch 'master' into move-manage-jenkins-less

commit 8b22270
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 8 16:19:42 2022 +0100

    Update theme.less

commit e925490
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jun 8 16:17:02 2022 +0100

    Add mixin for items

commit 0c8d538
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Jun 4 00:15:05 2022 +0100

    Update button styling

commit 45c9d07
Merge: e9cb78a 77a36fc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Jun 3 23:38:49 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit d579dc5
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue May 17 00:52:29 2022 +0100

    Update section.less

commit 43e4f5c
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue May 17 00:29:22 2022 +0100

    Init

commit ac594f3
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Apr 20 21:57:46 2022 +0100

    Update new node/view page button

commit df2a1af
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Apr 20 21:49:56 2022 +0100

    Update form.jelly

commit c33a9dc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Apr 20 21:46:32 2022 +0100

    Initial

commit e9cb78a
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Apr 20 21:01:06 2022 +0100

    Add backplate to table size toggles

commit 8cbfe00
Merge: 41d45b5 b28d225
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Apr 20 20:30:39 2022 +0100

    Merge branch 'master' into new-buttons-1-dashboard

commit 41d45b5
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 18 12:50:17 2022 +0100

    Initial
@janfaracik
Copy link
Contributor Author

Opened ATH here - jenkinsci/acceptance-test-harness#960

@NotMyFault NotMyFault added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Oct 2, 2022
@NotMyFault NotMyFault requested a review from a team October 2, 2022 21:46
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.

not tested yet

core/src/main/resources/lib/form/submit.jelly Outdated Show resolved Hide resolved
@janfaracik janfaracik changed the title draft: Update submit buttons to use .jenkins-button classes Update submit buttons to use .jenkins-button classes Oct 2, 2022
@siddoinghisjob
Copy link

Is this open? I would like to work on this.

Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Tested, looks OK to me in general.

Couple of oddities I've noticed, not sure if you had intent to cover those in this PR:

  • "OK" button on "New Item" page is not covered.
  • Navigating to Manage -> Nodes -> New node, Save button does not have min width set.

core/src/main/resources/lib/form/apply/apply.js Outdated Show resolved Hide resolved
@NotMyFault
Copy link
Member

For the first bullet point, I'd like to mention #7191; that's not a submit button, hence addressed separately.

@yaroslavafenkin
Copy link
Contributor

For the first bullet point, I'd like to mention #7191; that's not a submit button, hence addressed separately.

And I actually reviewed that... 🤦‍♂️

@janfaracik
Copy link
Contributor Author

janfaracik commented Oct 7, 2022

  • Navigating to Manage -> Nodes -> New node, Save button does not have min width set.

It does on mine?

image

Edit: Unless you meant the edit node page, which doesn't. I've updated it :)

Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Unless you meant the edit node page

Yup, meant that one.

Re-tested, LGTM now. Still not sure about the if condition magic though.

core/src/main/resources/lib/form/apply/apply.js Outdated Show resolved Hide resolved
@timja timja requested a review from yaroslavafenkin October 13, 2022 19:54
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.

Tested, LGTM

@timja timja requested a review from a team October 14, 2022 21:51
@NotMyFault NotMyFault requested a review from a team October 14, 2022 21:54
@timja
Copy link
Member

timja commented Oct 19, 2022

@yaroslavafenkin was the LGTM an approval? You didn't update the labels

or @jenkinsci/core-security-review

@yaroslavafenkin
Copy link
Contributor

yaroslavafenkin commented Oct 26, 2022

@yaroslavafenkin was the LGTM an approval? You didn't update the labels

or @jenkinsci/core-security-review

At that time I still had concerns about change in lib/form/apply/apply.js but it got reverted.

@yaroslavafenkin yaroslavafenkin 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 Oct 26, 2022
@timja
Copy link
Member

timja commented Oct 26, 2022

/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 Oct 26, 2022
@NotMyFault NotMyFault merged commit 5870838 into jenkinsci:master Oct 29, 2022
@basil
Copy link
Member

basil commented Nov 11, 2022

Causes hudson.plugins.throttleconcurrents.ThrottleQueueTaskDispatcherTest to start failing with e.g.

java.lang.AssertionError: Failed to find an input with type submit on the page
        at org.junit.Assert.fail(Assert.java:89)
        at hudson.plugins.throttleconcurrents.ThrottleQueueTaskDispatcherTest.submitForm(ThrottleQueueTaskDispatcherTest.java:347)
        at hudson.plugins.throttleconcurrents.ThrottleQueueTaskDispatcherTest.configureNewNodeWithLabel(ThrottleQueueTaskDispatcherTest.java:303)
        at hudson.plugins.throttleconcurrents.ThrottleQueueTaskDispatcherTest.assertBasedOnMaxLabelPairMatchingOrNot(ThrottleQueueTaskDispatcherTest.java:178)
        at hudson.plugins.throttleconcurrents.ThrottleQueueTaskDispatcherTest.testShouldConsiderTaskAsBlockableStillUponMatchingLabelPairWithLowestMax(ThrottleQueueTaskDispatcherTest.java:123)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
        at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:618)
        at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
        at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.lang.Thread.run(Thread.java:829)

basil added a commit to basil/throttle-concurrent-builds-plugin that referenced this pull request Nov 11, 2022
basil added a commit to jenkinsci/throttle-concurrent-builds-plugin that referenced this pull request Nov 11, 2022
@timja timja deleted the new-submit-buttons branch November 11, 2022 22:32
@daniel-beck
Copy link
Member

daniel-beck commented Nov 18, 2022

Caused JENKINS-70112.

@daniel-beck
Copy link
Member

Caused JENKINS-70662.

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

7 participants