[JENKINS-43786] More compatible adaptation #179
Conversation
| 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"))) { |
There was a problem hiding this comment.
2.88 is only a tentative version, I'm waiting for getting this jenkinsci/jenkins#2857 already merged
There was a problem hiding this comment.
Why do you need to wait that?
alert and alert-warning is available in Jenkins 1.633 (jenkinsci/ssh-agents-plugin#70 (review))
There was a problem hiding this comment.
what if it will be lts? Are side effects worse it?
There was a problem hiding this comment.
@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.
| 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"))) { |
There was a problem hiding this comment.
Why do you need to wait that?
alert and alert-warning is available in Jenkins 1.633 (jenkinsci/ssh-agents-plugin#70 (review))
| text(_('hook.registering.problem')) | ||
| f.submit(name: 'yes', value: _('view')) | ||
| f.submit(name: 'no', value: _('dismiss')) | ||
| } |
There was a problem hiding this comment.
Why do you want to put text to form?
(and not for Jenkins >= 2.88?)
There was a problem hiding this comment.
Originally text(_('hook.registering.problem')) was invoked after the form clause. It should be restored if there is no specific reason to do that
There was a problem hiding this comment.
@oleg-nenashev No, there is an important reason for that. The DOM generated should be synced with the proposal on jenkinsci/jenkins#2857
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@oleg-nenashev Please, could you update your review?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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')) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
@recena |
| text(_('hook.registering.problem')) | ||
| f.submit(name: 'yes', value: _('view')) | ||
| f.submit(name: 'no', value: _('dismiss')) | ||
| } |
There was a problem hiding this comment.
Originally text(_('hook.registering.problem')) was invoked after the form clause. It should be restored if there is no specific reason to do that
JENKINS-43786
Downstream of jenkinsci/jenkins#2857
@KostyaSha, @lanwen, What do you think?
This change is