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-19142] Prompt user whether to add the job to the current view #2529

Merged
merged 2 commits into from Sep 9, 2016

Conversation

5 participants
@Vlatombe
Copy link
Member

Vlatombe commented Aug 31, 2016

https://issues.jenkins-ci.org/browse/JENKINS-19142

@jenkinsci/code-reviewers

This adds a checkbox in the new job screen, allowing the user to control whether the job will be added to the current view.

jenkins-37857

@daniel-beck daniel-beck changed the title [JENKINS-37857] Prompt user whether to add the job to the current view [JENKINS-19142] Prompt user whether to add the job to the current view Aug 31, 2016

@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Aug 31, 2016

what if the current view is based on a regex filter?

@Vlatombe

This comment has been minimized.

Copy link
Member Author

Vlatombe commented Aug 31, 2016

If a regex is set then the checkbox will be unticked by default (see isAddToCurrentView method), unless if there are also jobs added through jobNames.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Sep 1, 2016

PR build on Firefox 48:

screen shot 2016-09-01 at 10 03 47

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Sep 1, 2016

Naming of the file is inconsistent with the vast majority of existing Jelly files.

~1/6 have dashes, about half of them to separate a prefix (config-) from the rest. Almost all others are lowercase or camelCase. Underscores are only used as prefix (e.g. _api.jelly).

I therefore recommend something like newJobButtonBar or newJob-buttonBar.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

Vlatombe commented Sep 1, 2016

@daniel-beck Both fixed

@batmat

This comment has been minimized.

Copy link
Member

batmat commented Sep 4, 2016

👍

I'm not sure I find the rule of checking/unchecking by default is definitely the right one. But as I can't come up with a better proposal, I'm fine with it.

If someone finds a better way, it'll be quite easy to adjust as Vincent isolated that well in a small decision method.

Also, used again docker run -ti -p 8080:8080 -e ID=2529 batmat/jenkins-pr-tester and happy to see it still works.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Sep 4, 2016

I think the defaults here are sensible. It's definitely a major improvement over the current situation.

Since the UI glitch has been resolved 👍

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Sep 9, 2016

👍

@oleg-nenashev oleg-nenashev merged commit f4c9eb0 into jenkinsci:master Sep 9, 2016

1 check passed

Jenkins This pull request looks good
Details

@Vlatombe Vlatombe deleted the Vlatombe:JENKINS-37857 branch Sep 9, 2016

daniel-beck added a commit that referenced this pull request Sep 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.