[JENKINS-43786] More compatible adaptation by recena · Pull Request #179 · jenkinsci/github-plugin

recena · 2017-10-29T18:34:48Z

JENKINS-43786

Downstream of jenkinsci/jenkins#2857

@KostyaSha, @lanwen, What do you think?

This change is

…nition

…nto JENKINS-43786

recena · 2017-10-29T18:35:56Z

...resources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy

-    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"))) {


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

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

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

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

ikedam

I can't get why this is related to JENKINS-43786 .

ikedam · 2017-10-29T23:05:33Z

...resources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy

-    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"))) {


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

ikedam · 2017-10-29T23:08:14Z

...resources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy

+            text(_('hook.registering.problem'))
+            f.submit(name: 'yes', value: _('view'))
+            f.submit(name: 'no', value: _('dismiss'))
+        }


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

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

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

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 ?

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

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

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

Fix for misplaced text and buttons. (this means there's a bug in the current version)

Preparation for [JENKINS-43786] Overhaul of Manage Jenkins page jenkins#2857 .

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.

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?

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

ikedam · 2017-10-29T23:12:17Z

...resources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy

    }
-    text(_('hook.registering.problem'))
-}
+}


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.

ikedam · 2017-10-29T23:22:36Z

@recena
Actually I can't get why you mentioned me as I haven't participate github-plugin nor JENKINS-43786.
You might have mistaken me for someone else to invite.

recena · 2017-10-30T08:24:44Z

@ikedam My apologies, I meant @lanwen.

oleg-nenashev

text(_('hook.registering.problem')) seems to be misplaced in the old UI

oleg-nenashev · 2017-10-30T08:42:58Z

...resources/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor/message.groovy

+            text(_('hook.registering.problem'))
+            f.submit(name: 'yes', value: _('view'))
+            f.submit(name: 'no', value: _('dismiss'))
+        }


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

recena added 4 commits September 14, 2017 21:51

[JENKINS-43786] Adapted the administrative monitor to the new UI defi…

8ab2318

…nition

[JENKINS-43786] Typo

e88b36e

Merge branch 'master' of https://github.com/jenkinsci/github-plugin i…

7d5a876

…nto JENKINS-43786

[JENKINS-43786] More compatible

22bb3fb

recena commented Oct 29, 2017

View reviewed changes

This was referenced Oct 29, 2017

[JENKINS-43786] Overhaul of Manage Jenkins page jenkinsci/jenkins#2857

Merged

[JENKINS-43786] Adapted the administrative monitor to the new UI definition jenkinsci/ssh-agents-plugin#70

Merged

ikedam suggested changes Oct 29, 2017

View reviewed changes

oleg-nenashev requested changes Oct 30, 2017

View reviewed changes

Conversation

recena commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanwen Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikedam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikedam commented Oct 29, 2017

Uh oh!

recena commented Oct 30, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

recena commented Oct 29, 2017 •

edited

Loading

lanwen Oct 30, 2017 •

edited

Loading