[JENKINS-34670] Add support for a new full screen layout #2445

recena · 2016-07-10T19:16:04Z

JENKINS-34670

Modify the Setup Wizard for using the full screen layout as a clear use-case.

The About page using the three different layout types (1024px x 768px).

full-screen: no header, no footer, no spaces

one-column: full width

two-column: side-panel + main-panel

@reviewbybees

ghost · 2016-07-10T19:31:04Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev · 2016-07-15T07:20:35Z

Looks good to me. 👍 and 🐝
By the way, it may a bit affect users, who bundle custom Javascripts to headers and footers. Maybe makes sense to keep empty header and footer tags then

recena · 2016-07-15T08:05:41Z

@oleg-nenashev This kind of things should not do. This layout is thought specially for:

Dashboard
To replace the page when Jenkins is restarting, and avoid the side-effect of resource not loaded
Maybe the InstallWizard (@kzantow)
etc...

oleg-nenashev · 2016-07-16T19:18:12Z

@reviewbybees done BTW
@jenkinsci/code-reviewers I would appreciate additional feedback

daniel-beck · 2016-07-17T00:31:25Z

I'm not sure this is a great change:

Isn't that what /lib/layout/html does? Why not just use that?
There's no rationale for this change. Why no headers/footers? I get that we may not want a side panel, but nothing?
How is one supposed to navigate away from such pages? The example About page shows it's a dead end with no way to return, seems like a messy, unusable approach.

My main concern is that piling on options in lib/layout for exceptional pages like those without headers/footers will result in maintenance problems later. Having a minimal(ish), separate /lib/layout/html seems preferable.

oleg-nenashev · 2016-07-17T09:57:31Z

Back to needs-review after the comment from @daniel-beck

kzantow · 2016-07-17T20:40:49Z

@daniel-beck /lib/layout/html should (and will) be removed in favor of this approach. the about page would use the one-column view, the no header/footer is for illustrative purposes only, which would replace the /lib/layout/html

🐝 from me on this change

daniel-beck · 2016-07-17T21:13:36Z

@kzantow

should … be removed in favor of this approach.

There's still no rationale for this change besides asserting it's the right thing to do. As I wrote,

piling on options in lib/layout for exceptional pages like those without headers/footers [may] result in maintenance problems later

recena · 2016-07-22T14:25:09Z

@reviewbybees done

ghost · 2016-07-22T14:33:04Z

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

recena · 2016-07-22T16:16:17Z

@daniel-beck It seems there are 3 persons, included the person who added the html.jelly, agree with this is the right way to implement several layouts (included the full screen).

recena · 2016-07-22T16:19:02Z

If I provided a way to include support for several layout types, was precisely to avoid hacks that we can see in Jenkins core and plugins.

daniel-beck · 2016-07-22T21:17:21Z

@recena

there are 3 persons [who agree] this is the right way to implement several layouts

And yet none of them managed to explain why it's a better approach, or even just addressed my specific concern above. There's only repeated assertions (another one from you just now) that this is the correct approach.

kzantow · 2016-07-22T21:52:37Z

@daniel-beck it's a better approach because it consolidates the style/script includes and resource references to a single file, instead of having to update 2 any time we might change them. And plugins could use this instead of defining local style overrides to hide the stuff, there's a common method of doing so.

daniel-beck · 2016-07-22T22:15:19Z

@kzantow

Thank you! I feel we're getting closer to a result.

it consolidates the style/script includes and resource references to a single file

This PR does not remove html.jelly, so there are no such benefits.

plugins could use this instead of defining local style overrides to hide the stuff

They can use html.jelly -- they may even already do so, because nowhere does it state that it's not fit for use from plugins.

So, to recap:

All the benefits of this change only apply if html.jelly gets removed.
html.jelly doesn't get removed.
In fact, it may be not possible to remove it, as it's effectively part of Jenkins' public plugin API now.

kzantow · 2016-07-22T22:19:23Z

@daniel-beck so, we could modify html.jelly to include the layout.jelly with the appropriate parameter to make it equivalent, probably remove the html.jelly usage in the setup wizard, and then figure out a way to deprecate it later.

recena · 2016-07-22T22:49:15Z

@daniel-beck

This is a step to remove html.helly and provide a right way to provide full-screen layout. This step belongs to a more general solution that started few months ago.

recena · 2016-07-22T22:50:13Z

@kzantow Thanks for your assistance. My internet connection is very limited on these days.

daniel-beck · 2016-07-22T23:08:00Z

@kzantow Good idea. I think this should be fine if it included that change so there's actually a benefit to merging this.

oleg-nenashev · 2016-08-08T08:12:22Z

So it's ready to merge from what I see

recena · 2016-08-08T08:27:00Z

@oleg-nenashev From my side it is ready to merge but I understand @daniel-beck wants to see a real usage of that. My recommendation is to prepare a new PR.

daniel-beck · 2016-08-08T10:05:43Z

@oleg-nenashev @recena I'd like to see actual applications of this code included in this same PR. Too much unused infrastructure in Jenkins as-is.

oleg-nenashev

I disagree with removal of lib/layout/html.jelly in such way. Even if it does not make sense for other usages (proof?), it was a part of the Jenkins API for Jelly, which is documented in Jenkins TagLib documentation. This TagLib site is not being hosted on jenkins.io now (hope it changes at some point), but IMHO it is not safe to just remove it.

I would propose to keepthis Jelly tag somehow. It is fine to mark it as deprecated in the documentation if you do not want to inherit from it like @daniel-beck proposed.

recena · 2017-03-15T20:22:04Z

THIS IS INCREDIBLE!

recena · 2017-03-15T20:24:19Z

@oleg-nenashev html.jelly was introduced by @kzantow in order to provide a full screen layout. This tag is being used only in the setup wizard.

recena · 2017-03-15T20:25:21Z

@oleg-nenashev Curiously, the author of this Jelly tag agrees with the changes proposed here.

recena · 2017-03-15T20:30:49Z

@oleg-nenashev

it was a part of the Jenkins API for Jelly,

The perfect argument for keeping legacy code for a long time

which is documented in Jenkins TagLib documentation.

And?

I would propose to keepthis Jelly tag somehow

Why? Where is being used?

if you do not want to inherit from it

Confused, not needed. layout.jelly was and is, the original propose for implementing different UI layouts.

recena · 2017-03-15T20:32:11Z

This PR was filed since Jul 10, 2016

recena · 2017-03-15T20:43:09Z

I'm remembering this one #1717, with more than 122 comments.

oleg-nenashev · 2017-03-16T10:00:00Z

I and @recena discussed this issue internally. I still disagree with such ranting and I still feel strong about my feedback.

I propose the following ways to proceed...

Get more votes for @jenkinsci/code-reviewers . I do not veto this PR (nobody can)
Decouple the html.jelly deletion to a separate PR an continue the discussion there

recena · 2017-03-16T10:09:45Z

If ranting means sharing / expressing my frustration, yes. Each change related with the UI involves a BIG challenge. We should think / reflect why the UI and UX have been one of the most negative part of this project.

…o JENKINS-34670

recena · 2017-03-31T16:51:18Z

@oleg-nenashev Given the two options, I've choose the second one.

recena · 2017-04-01T15:38:32Z

@oleg-nenashev

I would propose to keep this Jelly tag somehow. It is fine to mark it as deprecated in the documentation if you do not want to inherit from it like @daniel-beck proposed.

This was what I did.

oleg-nenashev

👍 for the code itself. Ready to merge assuming it was manually tested

oleg-nenashev · 2017-04-01T17:34:17Z

@daniel-beck wdyt?

recena · 2017-04-01T18:24:22Z

@oleg-nenashev I'm going to prepare the PR for removing html.jelly, as you suggested.

…NKINS-34670

recena · 2017-04-01T19:44:33Z

@oleg-nenashev this was tested many times.

Today, I did smoke tests against: 2.32.3, 2.41.1 and master.

recena · 2017-04-01T19:45:58Z

By the way, I did a rebase with master, this was my last commit.

oleg-nenashev · 2017-04-01T19:47:59Z

@recena OK, ty. Will sync-up on the release of this fix with Daniel

oleg-nenashev · 2017-04-02T18:22:43Z

According to the feedback from @daniel-beck , he is neutral regarding this change.
Merging according to the feedback from others

oleg-nenashev · 2017-04-02T18:45:12Z

My bad, merged it without squash. But it is not a backporting candidate anyway

recena · 2017-04-02T18:54:25Z

@oleg-nenashev In general, I prefer to merge this kind of changes by using squash mode. But as you mentioned, this is not a candidate for backporting.

recena · 2017-04-02T18:55:00Z

@kzantow @oleg-nenashev @daniel-beck I appreciate a lot your time and reviews.

[JENKINS-34670] Add support for a new full screen layout

fe07641

oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jul 15, 2016

oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 17, 2016

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Aug 8, 2016

oleg-nenashev requested changes Mar 15, 2017

View reviewed changes

recena added 2 commits March 31, 2017 17:21

[JENKINS-34670] Reverting html.jelly and noting as deprecated

c8970db

Merge branch 'JENKINS-34670' of https://github.com/recena/jenkins int…

bb1ff6a

…o JENKINS-34670

oleg-nenashev approved these changes Apr 1, 2017

View reviewed changes

Merge branch 'master' of https://github.com/jenkinsci/jenkins into JE…

f3f29c0

…NKINS-34670

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Apr 2, 2017

oleg-nenashev merged commit cc85dc1 into jenkinsci:master Apr 2, 2017

recena deleted the JENKINS-34670 branch April 2, 2017 18:55

recena mentioned this pull request Apr 20, 2017

Removed html.jelly since it was deprecated as of 2.53 #2852

Closed

jglick mentioned this pull request Dec 20, 2024

headerCommandPaletteButton undefined when JenkinsHeader not loaded #10089

Merged

[JENKINS-34670] Add support for a new full screen layout #2445

[JENKINS-34670] Add support for a new full screen layout #2445

Conversation

recena commented Jul 10, 2016 • edited Loading

full-screen: no header, no footer, no spaces

one-column: full width

two-column: side-panel + main-panel

ghost commented Jul 10, 2016

oleg-nenashev commented Jul 15, 2016

recena commented Jul 15, 2016

oleg-nenashev commented Jul 16, 2016

daniel-beck commented Jul 17, 2016 • edited Loading

oleg-nenashev commented Jul 17, 2016

kzantow commented Jul 17, 2016

daniel-beck commented Jul 17, 2016

recena commented Jul 22, 2016

ghost commented Jul 22, 2016

recena commented Jul 22, 2016

recena commented Jul 22, 2016

daniel-beck commented Jul 22, 2016

kzantow commented Jul 22, 2016

daniel-beck commented Jul 22, 2016

kzantow commented Jul 22, 2016

recena commented Jul 22, 2016

recena commented Jul 22, 2016

daniel-beck commented Jul 22, 2016

oleg-nenashev commented Aug 8, 2016

recena commented Aug 8, 2016

daniel-beck commented Aug 8, 2016

oleg-nenashev left a comment

Choose a reason for hiding this comment

recena commented Mar 15, 2017

recena commented Mar 15, 2017 • edited Loading

recena commented Mar 15, 2017 • edited Loading

recena commented Mar 15, 2017

recena commented Mar 15, 2017

recena commented Mar 15, 2017

oleg-nenashev commented Mar 16, 2017

recena commented Mar 16, 2017 • edited Loading

recena commented Mar 31, 2017

recena commented Apr 1, 2017 • edited Loading

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Apr 1, 2017

recena commented Apr 1, 2017 • edited Loading

recena commented Apr 1, 2017

recena commented Apr 1, 2017 • edited Loading

oleg-nenashev commented Apr 1, 2017

oleg-nenashev commented Apr 2, 2017

oleg-nenashev commented Apr 2, 2017

recena commented Apr 2, 2017

recena commented Apr 2, 2017

recena commented Jul 10, 2016 •

edited

Loading

daniel-beck commented Jul 17, 2016 •

edited

Loading

recena commented Mar 15, 2017 •

edited

Loading

recena commented Mar 15, 2017 •

edited

Loading

recena commented Mar 16, 2017 •

edited

Loading

recena commented Apr 1, 2017 •

edited

Loading

recena commented Apr 1, 2017 •

edited

Loading

recena commented Apr 1, 2017 •

edited

Loading