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-43786] Overhaul of Manage Jenkins page #2857

Merged
merged 33 commits into from Jan 21, 2018

Conversation

@recena
Copy link
Contributor

recena commented Apr 23, 2017

Description

See JENKINS-43786.

  • Apply a semantic HTML
  • Remove the <table> tag usage for implementing layouts and content structures. If you need reasons or arguments.
  • Small re-style focused on spacing, margins, etc...
  • Accesibility

Screenshots

There are more screenshots along the PR comments.

Before

jenkins-43786_before-1

jenkins-43786_before-2

After

manage jenkins jenkins

captura el 2017-05-08 a las 20 32 41

jenkins-43786_after-1

Changelog entries

Proposed changelog entries:

  • Re-styled the Manage Jenkins page, including the administrative monitors.

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • An existing test was reviewed

Desired reviewers

@jenkinsci/code-reviewers

@@ -68,12 +67,75 @@ THE SOFTWARE.

<st:include page="downgrade.jelly" />

<table style="padding-left: 2em;" id="management-links">
<style type="text/css">

This comment has been minimized.

Copy link
@recena

recena Apr 23, 2017

Author Contributor

The final definition will be placed on the proper CSS file.

</j:forEach>


<table style="padding-left: 2em; display: none;" id="management-links">

This comment has been minimized.

Copy link
@recena

recena Apr 23, 2017

Author Contributor

It will be removed. It is still here for testing purposes.

This comment has been minimized.

Copy link
@recena

recena Jul 9, 2017

Author Contributor

Given this block of code will be removed, this functional test needs to reviewed.

This comment has been minimized.

Copy link
@recena

recena Jul 9, 2017

Author Contributor

And maybe this code.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Apr 23, 2017

Do we need ATH runs for this?

@daniel-beck daniel-beck changed the title [JENKINS-43786][WiP] Initial source code modifications [JENKINS-43786][WiP] Overhaul of Manage Jenkins page Apr 23, 2017
@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Apr 24, 2017

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Apr 24, 2017

@recena What do you mean? I'm aware.

daniel-beck added the work-in-progress label 9 hours ago

@olivergondza

This comment has been minimized.

Copy link
Member

olivergondza commented Apr 24, 2017

I do not think ATH has much of a coverage for this few things this can possibly break.

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Apr 25, 2017

Please provide before and after screenshot if it visible UI change.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Apr 25, 2017

FWIW I like the "hover" design in the screenshot a lot.

Ideally we'd make this a reusable component, so e.g. lists like new agent/view could use the same design.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 6, 2017

@KostyaSha I'll do it. To be honest, this is a basic re-style but IMHO improves some small details. Thanks for your participation.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented May 6, 2017

@daniel-beck Let me finish the approach and then, we can see the level of re-utilization.

@@ -39,7 +39,7 @@ SystemInfoLink.DisplayName=System Information
SystemInfoLink.Description=Displays various environmental information to assist trouble-shooting.

SystemLogLink.DisplayName=System Log
SystemLogLink.Description=System log captures output from <tt>java.util.logging</tt> output related to Jenkins.
SystemLogLink.Description=System log captures output from <code>java.util.logging</code> output related to Jenkins.

This comment has been minimized.

Copy link
@recena

recena May 6, 2017

Author Contributor

Unrelated but <tt> is a obsolete element. Oldschool 😄

@@ -35,8 +35,7 @@ THE SOFTWARE.
${taskTags!=null and attrs.contextMenu!='false' ? taskTags.add(href,iconUrl,title,post,requiresConfirmation) : null}
<!-- TODO summary.jelly should be modified to accept requiresConfirmation so the icon link can be included -->
<j:set var="_href" value="${href}"/>
<t:summary icon="${icon}"
href="${requiresConfirmation || post ? null : href}" iconOnly="true">
<t:summary icon="${icon}" href="${requiresConfirmation || post ? null : href}" iconOnly="true">

This comment has been minimized.

Copy link
@recena

recena May 6, 2017

Author Contributor

Easier to read.

<j:forEach var="m" items="${it.managementLinks}">
<l:hasPermission permission="${m.requiredPermission}">
<j:set var="icon" value="${m.iconClassName != null ? m.iconClassName : m.iconFileName}"/>
<j:if test="${icon!=null}">
<local:feature icon="${icon}" href="${m.urlName}" title="${m.displayName}" requiresConfirmation="${m.requiresConfirmation}" post="${m.requiresPOST}">
<local:feature icon="${icon}" href="${m.urlName}" title="${m.displayName}" requiresConfirmation="${m.requiresConfirmation}" post="${m.requiresPOST}">

This comment has been minimized.

Copy link
@recena

recena May 6, 2017

Author Contributor

Unnecessary spaces.

This comment has been minimized.

Copy link
@recena

recena Jul 9, 2017

Author Contributor

@daniel-beck Do you know any case where <local:feature> is involved? I'd like to include this part in this re-style. Thanks.

This comment has been minimized.

Copy link
@recena

recena Jul 13, 2017

Author Contributor

@oleg-nenashev And you? Could you help me?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 15, 2017

Member

@recena local:feature is a local taglib defined directly in manage.jelly. https://github.com/recena/jenkins/blob/a4b4771a90484b8326fafea92fc47921492cd8a2/core/src/main/resources/jenkins/model/Jenkins/manage.jelly#L32-L55

AFAIK it cannot be used elsewhere outside this file

This comment has been minimized.

Copy link
@recena

recena Jul 15, 2017

Author Contributor

@oleg-nenashev What I really wanted was to know how exercising this taglib. Some use-case.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jul 15, 2017

Member

@recena Reusability for less repetition, see https://github.com/recena/jenkins/blob/6d119ef16946312d8c4ca319babb2e2b292b9b79/core/src/main/resources/jenkins/model/Jenkins/manage.jelly.

The October 2012 changes converted those to ManagementLinks in one loop, so it's not relevant anymore, but it was before then.

This comment has been minimized.

Copy link
@recena

recena Jul 16, 2017

Author Contributor

@daniel-beck I appreciate your feedback. Can I understand we could make a cleanup?

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jul 16, 2017

Member

Assuming by cleanup you mean inlining the tag, yes.

@@ -155,9 +155,9 @@ public HttpResponse doForward(@QueryParameter String fix, @QueryParameter String
}

/**
* Returns true iff there are applicable but ignored (i.e. hidden) warnings.
* Returns true if there are applicable but ignored (i.e. hidden) warnings.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck May 30, 2017

Member

These are not typos.

iff is shorthand for if and only if, see e.g. http://www.dictionary.com/browse/iff

This comment has been minimized.

Copy link
@recena

recena Jul 3, 2017

Author Contributor

My bad English. Thanks for noticing it.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented May 30, 2017

In 8774f3c PluginWrapperAdministrativeMonitor/message.jelly is whitespace only, changing indentation, but Jenkins/manage.jelly does not indent newly wrapped elements (presumably to keep whitespace changes minimal)? Why?

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented May 30, 2017

What is the benefit of these ~1500 lines of (minified) Bootstrap?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 17, 2017

@recena Any plans to continue working on it?

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jul 3, 2017

@daniel-beck A customized version of bootstrap including some needed things. My proposal is to include Bootstrap by default in Jenkins as part of the core.

@oleg-nenashev Yes, this is my plan.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jul 28, 2017

screenshot-localhost 8080 2017-07-28 18-48-00

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Jul 28, 2017

@recena How does this look with multiple multiple admin monitors, how does this affect the warnings popup shown on all other pages?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 7, 2018

We still need a blogpost draft to get it merged.
The delay with the blogpost proves that it is a good idea to require ready pull requests with blog posts as prerequisites for merging

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 7, 2018

Sure. Working on it.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 15, 2018

I think we could remove the label needs-docs since the blogpost was written.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 15, 2018

@jenkinsci/code-reviewers back to review

@bitwiseman

This comment has been minimized.

Copy link
Contributor

bitwiseman commented Jan 16, 2018

@recena @daniel-beck - Is the blog post sufficient to say this has been documented? A blog post is great but is there any other actual documentation that need to change?

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 16, 2018

I don't think so. My commitment was to write a blog post in order to spread the word about this improvement.

@bitwiseman

This comment has been minimized.

Copy link
Contributor

bitwiseman commented Jan 16, 2018

@recena - Understood. 😄 I wasn't trying to shift the scope of your work.

I meant to ask those driving developer documentation such as @daniel-beck @oleg-nenashev, when you say "needs-docs" are you comfortable with that being just a blog post?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 16, 2018

@bitwiseman I am fine with that, the blogpost just needs to be linked from changelog.

Regarding the change, I cannot guarantee to review it this week. It depends on how serious the JEP-200 fallout is. Keeps me busy so far. Anybody else can merge it and orchestrate the delivery, but I suggest to wait till Thursday. We may need an out-of-order release tomorrow

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 16, 2018

Thanks for your feedback. I hope to see these changes merged in the upcoming normal weekly. I'm planning next steps.

@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 20, 2018

Will I be luck on this weekly release? 😄

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 20, 2018

Sorry, I had no time to review PRs this week. I will try to review it tomorrow on the morning

@oleg-nenashev oleg-nenashev removed the on-hold label Jan 20, 2018
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 20, 2018

IMO it is ready to go. The blogpost needs the date update, but it's ready in other matters

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 20, 2018

@recena would you be fine if I squash the PR during the merge? I lean towards that since there are many small changes like whitespace ones, but there is no strict policy

@oleg-nenashev oleg-nenashev self-assigned this Jan 20, 2018
@recena

This comment has been minimized.

Copy link
Contributor Author

recena commented Jan 21, 2018

@oleg-nenashev Sure. My personal preference is always to squash.

@oleg-nenashev oleg-nenashev merged commit 6f2769e into jenkinsci:master Jan 21, 2018
1 check passed
1 check passed
continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 21, 2018

merged

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Jan 24, 2018

Probably caused JENKINS-49129.

<!-- table to show a map -->
<d:tag name="feature">
<j:set var="iconUrl" value="${icon.startsWith('/') ? resURL+icon : imagesURL+'/48x48/'+icon}"/>
${taskTags!=null and attrs.contextMenu!='false' ? taskTags.add(href,iconUrl,title,post,requiresConfirmation) : null}

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jan 24, 2018

Member

This line was removed without replacement.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Jan 24, 2018

Member

Indeed it looks like taskTags is involved with context menu creation, see ModelObjectWithContextMenu.java.

abayer added a commit to abayer/script-security-plugin that referenced this pull request Apr 17, 2018
jenkinsci/jenkins#2857 reworked the management
page, so that from core 2.103 onward, there's only one
`scriptApproval` link where there had been two. So only look for 1
link in relevant core versions while still looking for two in earlier versions.
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.