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.

Sign up for GitHub

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-43786] More compatible adaptation #179

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[JENKINS-43786] More compatible adaptation #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ab2318
[JENKINS-43786] Adapted the administrative monitor to the new UI defi…
recena Sep 14, 2017
e88b36e
[JENKINS-43786] Typo
recena Oct 29, 2017
7d5a876
Merge branch 'master' of https://github.com/jenkinsci/github-plugin i…
recena Oct 29, 2017
22bb3fb
[JENKINS-43786] More compatible
recena Oct 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions ...ources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,24 @@
package org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor

import hudson.util.VersionNumber
import jenkins.model.Jenkins

def f = namespace(lib.FormTagLib)

div(class: 'alert alert-warning') {
form(method: 'post', action: "${rootURL}/${my?.url}/act", name: my?.id) {
f.submit(name: 'yes', value: _('view'))
f.submit(name: 'no', value: _('dismiss'))
if (Jenkins.getVersion().isNewerThan(new VersionNumber("2.88"))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2.88 is only a tentative version, I'm waiting for getting this jenkinsci/jenkins#2857 already merged

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to wait that?
alert and alert-warning is available in Jenkins 1.633 (jenkinsci/ssh-agents-plugin#70 (review))

Copy link
Member

@lanwen lanwen Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

what if it will be lts? Are side effects worse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lanwen I don't think so. With this approach if the plugin is running on Jenkins version < 2.88 we see the current appearance, but if the plugin is running on Jenkins > 2.88 we see the new design.

div(class: 'alert alert-warning') {
form(method: 'post', action: "${rootURL}/${my?.url}/act", name: my?.id) {
f.submit(name: 'yes', value: _('view'))
f.submit(name: 'no', value: _('dismiss'))
}
text(_('hook.registering.problem'))
}
} else {
div(class: 'warning') {
form(method: 'post', action: "${rootURL}/${my?.url}/act", name: my?.id) {
text(_('hook.registering.problem'))
f.submit(name: 'yes', value: _('view'))
f.submit(name: 'no', value: _('dismiss'))
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to put text to form?
(and not for Jenkins >= 2.88?)

Copy link
Member

Choose a reason for hiding this comment

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

Originally text(_('hook.registering.problem')) was invoked after the form clause. It should be restored if there is no specific reason to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev No, there is an important reason for that. The DOM generated should be synced with the proposal on jenkinsci/jenkins#2857

Copy link
Member

Choose a reason for hiding this comment

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

This else block is for Jenkins <= 2.88, that is, without jenkinsci/jenkins#2857 .
Do you mean this DOM should be fixed regardless of jenkinsci/jenkins#2857 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ikedam Yes, jenkinsci/jenkins#2857 provides a new re-styling and if a maintainer wants to use it, needs to perform a small change like this. But we want to keep backward compatibility in the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev Please, could you update your review?

Copy link
Member

Choose a reason for hiding this comment

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

@recena
I'm afraid you didn't get the point I asked.
I suppose this PR contains 2 changes:

You haven't mentioned the prior one, and it makes me wonder whether that change is expected one, or the one by mistake.

Attaching screenshot might clarify the fix by this PR.

Copy link
Member

Choose a reason for hiding this comment

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

No, there is an important reason for that. The DOM generated should be synced with the proposal on ...

It is for the legacy branch. Why does it need to be synced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev I don't know what you are not understanding. There are two blocks, one with the original implementation and other with the new one. And yes, the order on how the elements are rendered is important.

}
text(_('hook.registering.problem'))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a line break.
Though no line break is almost harmless for text editors, text files must end with line breaks for the POSIX definition.

2 changes: 1 addition & 1 deletion ...es/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
view=View
dismiss=Dismiss
hook.registering.problem=There were some problems while registering or removing one ore more GitHub webhooks. Would you like to view the problems?
hook.registering.problem=There were some problems while registering or removing one or more GitHub webhooks. Would you like to view the problems?