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-43581] - Create new custom tool button should be on top #2926

Merged
merged 8 commits into from Mar 4, 2018

Conversation

5 participants
@Ykus
Contributor

Ykus commented Jun 25, 2017

See JENKINS-43581.

Proposed changelog entries

  • JENKINS-43581 - RFE - Show the "Add" button on the top for Tool Installations
  • JENKINS-43581 - RFE - Internal: Add the enableTopButton attribute to lib/form/repeatable Jelly control. If enabled, the "Add" button will be also shown on the top

Adds button, for adding new entries, on top of a repeatable form (issue 43581)

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@oleg-nenashev

Ykus added some commits May 27, 2017

@jenkinsci jenkinsci deleted a comment from reviewbybees Jun 26, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jun 26, 2017

@Ykus Please do not use @reviewbybees if you not a CloudBees employee. Use @jenkinsci/code-reviewers instead

@oleg-nenashev

The change impacts all repeatable.jelly usages, and IMHO it may cause some UX issues in cases when the list is short.

I would advice to add a new attrs parameter, which enables the button on the top. And then apply it to the long layouts like tools.

The code itself looks good to me, but I am not a Javascript developer. Extra feedback would be useful

@Ykus

This comment has been minimized.

Contributor

Ykus commented Jul 9, 2017

@oleg-nenashev I have changed repeatable forms behavior regarding your suggestion.

@oleg-nenashev

I have reviewed it yesterday. The most of the change looks good, and thanks for adding tests for it. 👍

I have only minor comments regarding the license headers and the CSS file

<!--
The MIT License
Copyright (c) 2004-2010, Sun Microsystems, Inc., Alan Harder

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 9, 2017

Member

I am not sure about this and other headers. Was it full copy-paste of the file? If no, makes sense to update dates and to add your names

@@ -1418,7 +1418,7 @@ DIV.to-be-removed { display: none; }
/* ========================= Other form related CSS ========================= */
.row-set-end { display: none; }
.row-set-end, .hide { display: none; }

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 9, 2017

Member

Hm, is it related? I see no usages

This comment has been minimized.

@Ykus

Ykus Jul 9, 2017

Contributor

Good catch, Thanks.

@@ -1,7 +1,7 @@
<!--
The MIT License
Copyright (c) 2004-2010, Sun Microsystems, Inc., Alan Harder
Copyright (c) 2004-2017 by Zdenek Soukup

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 9, 2017

Member

2017- then

@@ -88,6 +88,10 @@ THE SOFTWARE.
true if the default 'add' button (that adds a new copy) shouldn't be displayed.
When you use this attribute,
</st:attribute>
<st:attribute name="enableTopButton">
true if the top default 'add' button (that adds a new copy) should be displayed.
Display of top button depends also on number of items.

This comment has been minimized.

@daniel-beck

daniel-beck Jul 9, 2017

Member

Too vague.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jul 9, 2017

Does this just move the button, or add a second button?

What sorts of lists in Jenkins does this affect? Could this also be applied to lists of build steps in freestyle projects? IIRC there is a request to add new steps on top.

@Ykus

This comment has been minimized.

Contributor

Ykus commented Jul 9, 2017

It adds new (second) button above the repeatable form.

It affects only repeatable (repeatable.js) lists. Freestyle projects are made by hetero-list. I think I can make similar changes for hetero-list as I made for repeatable.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jul 22, 2017

I think @daniel-beck should just create a follow-up ticket for the Hetero List. This is also a valuable change, but it seems to be out of the scope. I have modified the changelog entries in order to reflect which control and page are updated.

@daniel-beck daniel-beck self-assigned this Jul 22, 2017

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jul 22, 2017

@oleg-nenashev Alike things should behave alike. It's insane that Jenkins records who created an agent, but not any other object in Jenkins.

I'd like to take another look at this before this goes in to understand it better.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jul 22, 2017

ack, on hold then

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 3, 2017

@daniel-beck gentle reminder. It would be nice to land it into the next LTS baseline, because it is a real problem for big tool configs

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Aug 4, 2017

I suppose it fixes the issue as reported by introducing yet more buttons. I feel sorry for the person who wants their stuff to appear in the middle of the list.

Besides that obvious limitation, I'm really not sure about this. As I wrote above,

Alike things should behave alike.

And all other lists don't get this. I'm pretty sure the corresponding request for build steps is even older than this. In the case of build steps, we might be saying that it's not a good idea to have so many different ones to begin with. But personally, I'd say the same for lists of tool installations.

Not strongly opposed, but I don't think this is a good solution either.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 4, 2017

And all other lists don't get this. I'm pretty sure the corresponding request for build steps is even older than this. In the case of build steps, we might be saying that it's not a good idea to have so many different ones to begin with

I agree though I rather see it as a follow-up ticket. Testing UX on a single control LGTM. More feedback from @jenkinsci/code-reviewers would be definitely useful

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 12, 2017

@jenkinsci/code-reviewers ping, the vacation period is almost over ;)

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 12, 2017

Unassigned @daniel-beck according to his feedback in IRC, we need more votes

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 4, 2017

@jenkinsci/code-reviewers ping. Custom Tools plugin is generally broken now due to the dependency on insecure Extended Choice version, but maybe somebody is up to reviewing it

@oleg-nenashev oleg-nenashev self-assigned this Nov 4, 2017

@recena

This comment has been minimized.

Contributor

recena commented Nov 4, 2017

IMHO, screenshots before/after are very helpful for this PR.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Nov 6, 2017

I agree though I rather see it as a follow-up ticket. Testing UX on a single control LGTM.

Then we need specific commitment of someone to follow up on this no later than N months from now and either revert, or implement everywhere.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 21, 2018

I won't commit to implement anything, because I cannot guarantee that I deliver on this commitment. And I do not get why this particular PR is being blocked by that @daniel-beck. I have recently received a provate ping from @Ykus about this proposal, but I cannot help much in this case. And I do not want to push a contributor to rework many Jenkins components just to make his limited-scope UI improvement landed.

@daniel-beck could you please work with @Ykus to agree on the action plan?

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 22, 2018

@oleg-nenashev

I do not think the minor usability improvement here outweighs the resulting inconsistency between similar UI controls, making this a (slightly) net negative change. Hence my previous comment.

The road forward is clear IMO: Not do this, or apply it consistently.

Again, I'm not specifically blocking this if you feel strongly, but it doesn't look to me that many others are interested in seeing this land based on limited feedback here.

@jglick

This comment has been minimized.

Member

jglick commented Jan 22, 2018

We are not generally investing time in “classic UI”; contributions should go toward Blue Ocean. Also the whole tool system ought to be considered semi-deprecated in favor of Docker agents. So I am not about to spend time reviewing this.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 4, 2018

I am merging it on my own responsibility since nobody else blocks the pull request. Although it is a "minor improvement" for API users, it is a significant UX improvement for Jenkins admins who have to define many tool installations. The change is well covered by tests. Since we have much bigger barely-justified changes like XML 1.1 merged, I consider it is as a judgment call. And in this case I decide to merge this contribution from a community member instead of blocking it indefinitely.

I have created JENKINS-49900 as a follow-up to @daniel-beck's requirement. It's a newbie-friendly ticket, I will be offering it to GSoC students who are interested in UI-related project ideas.

@oleg-nenashev oleg-nenashev merged commit d9e1f38 into jenkinsci:master Mar 4, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 4, 2018

Thanks for your contribution @Ykus! My apologies for it taking so long

@recena

This comment has been minimized.

Contributor

recena commented Mar 4, 2018

@jglick

We are not generally investing time in “classic UI”; contributions should go toward Blue Ocean.

Long life to the classic UI 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment