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

recena · 2017-04-23T10:04:01Z

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

After

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

recena · 2017-04-23T10:05:42Z

core/src/main/resources/jenkins/model/Jenkins/manage.jelly

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

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

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


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

recena · 2017-04-23T10:06:35Z

core/src/main/resources/jenkins/model/Jenkins/manage.jelly

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


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

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

And maybe this code.

daniel-beck · 2017-04-23T23:36:47Z

Do we need ATH runs for this?

recena · 2017-04-24T07:56:50Z

@daniel-beck WiP

daniel-beck · 2017-04-24T09:03:47Z

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

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

olivergondza · 2017-04-24T13:10:28Z

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

KostyaSha · 2017-04-25T01:50:56Z

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

daniel-beck · 2017-04-25T18:05:55Z

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 · 2017-05-06T09:17:34Z

@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 · 2017-05-06T09:19:38Z

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

recena · 2017-05-06T09:30:21Z

core/src/main/resources/jenkins/management/Messages.properties

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


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

recena · 2017-05-06T09:30:44Z

core/src/main/resources/jenkins/model/Jenkins/manage.jelly

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


Easier to read.

recena · 2017-05-06T09:31:21Z

core/src/main/resources/jenkins/model/Jenkins/manage.jelly

      <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}">


Unnecessary spaces.

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

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

@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

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

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

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

Assuming by cleanup you mean inlining the tag, yes.

…es were adapted

…NKINS-43786

daniel-beck · 2017-05-30T15:11:16Z

core/src/main/java/jenkins/security/UpdateSiteWarningsMonitor.java

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


These are not typos.

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

My bad English. Thanks for noticing it.

daniel-beck · 2017-05-30T15:13:09Z

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 · 2017-05-30T15:14:11Z

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

oleg-nenashev · 2017-06-17T13:48:02Z

@recena Any plans to continue working on it?

recena · 2017-07-03T20:47:11Z

@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 · 2017-07-28T16:51:11Z

daniel-beck · 2017-07-28T22:22:43Z

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

oleg-nenashev · 2018-01-07T13:45:59Z

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 · 2018-01-07T13:52:06Z

Sure. Working on it.

recena · 2018-01-15T18:31:08Z

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

oleg-nenashev · 2018-01-15T18:33:58Z

@jenkinsci/code-reviewers back to review

bitwiseman · 2018-01-16T21:02:52Z

@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 · 2018-01-16T21:22:22Z

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

bitwiseman · 2018-01-16T21:46:02Z

@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 · 2018-01-16T21:56:51Z

@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 · 2018-01-16T22:15:47Z

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

recena · 2018-01-20T09:29:35Z

Will I be luck on this weekly release? 😄

oleg-nenashev · 2018-01-20T20:38:37Z

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

oleg-nenashev · 2018-01-20T21:04:13Z

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

oleg-nenashev · 2018-01-20T21:06:02Z

@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

recena · 2018-01-21T11:45:21Z

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

oleg-nenashev · 2018-01-21T13:29:00Z

merged

daniel-beck · 2018-01-24T10:36:43Z

Probably caused JENKINS-49129.

daniel-beck · 2018-01-24T14:37:18Z

core/src/main/resources/jenkins/model/Jenkins/manage.jelly

-    <!-- 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 line was removed without replacement.

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

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.

[JENKINS-43786] Initial source code modifications

7dbaf5f

recena commented Apr 23, 2017

View reviewed changes

daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Apr 23, 2017

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

recena commented May 6, 2017

View reviewed changes

recena added 3 commits May 6, 2017 14:09

[JENKINS-43786] Adding a customized Bootstrap version and some messag…

52654eb

…es were adapted

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

d8b4526

…NKINS-43786

[JENKINS-43786] Some Monitors included in the core have been adapted

8774f3c

daniel-beck reviewed May 30, 2017

View reviewed changes

recena added 4 commits July 9, 2017 14:43

[JENKINS-43786] Rebasing with master

beaf805

[JENKINS-43786] Reverting an unexisting typo

7dd0429

[JENKINS-43786] Applying final styles

a4b4771

[JENKINS-43786] Addressing more use cases

427ab29

batmat removed their request for review December 15, 2017 08:23

recena mentioned this pull request Jan 14, 2018

[JENKINS-43786] Blog post about JENKINS-43786 jenkins-infra/jenkins.io#1327

Merged

oleg-nenashev removed the needs-docs label Jan 15, 2018

oleg-nenashev removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jan 20, 2018

oleg-nenashev self-assigned this Jan 20, 2018

oleg-nenashev merged commit 6f2769e into jenkinsci:master Jan 21, 2018

daniel-beck reviewed Jan 24, 2018

View reviewed changes

abayer mentioned this pull request Apr 17, 2018

Fix approval link count check for newer core versions jenkinsci/script-security-plugin#200

Merged

daniel-beck mentioned this pull request Oct 7, 2019

[JENKINS-59684] Support "alert-info" for admin monitor in popup #4276

Merged

2 tasks

		</j:forEach>


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

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

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

Conversation

recena commented Apr 23, 2017 • edited

Description

Screenshots

Before

After

Changelog entries

Submitter checklist

Desired reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recena Jul 9, 2017 • edited

Choose a reason for hiding this comment

daniel-beck commented Apr 23, 2017

recena commented Apr 24, 2017

daniel-beck commented Apr 24, 2017

olivergondza commented Apr 24, 2017 • edited

KostyaSha commented Apr 25, 2017 • edited

daniel-beck commented Apr 25, 2017

recena commented May 6, 2017

recena commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented May 30, 2017

daniel-beck commented May 30, 2017

oleg-nenashev commented Jun 17, 2017

recena commented Jul 3, 2017

recena commented Jul 28, 2017

daniel-beck commented Jul 28, 2017

oleg-nenashev commented Jan 7, 2018

recena commented Jan 7, 2018

recena commented Jan 15, 2018

oleg-nenashev commented Jan 15, 2018

bitwiseman commented Jan 16, 2018

recena commented Jan 16, 2018

bitwiseman commented Jan 16, 2018 • edited

oleg-nenashev commented Jan 16, 2018

recena commented Jan 16, 2018

recena commented Jan 20, 2018 • edited

oleg-nenashev commented Jan 20, 2018

oleg-nenashev commented Jan 20, 2018

oleg-nenashev commented Jan 20, 2018

recena commented Jan 21, 2018

oleg-nenashev commented Jan 21, 2018

daniel-beck commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recena commented Apr 23, 2017 •

edited

recena Jul 9, 2017 •

edited

olivergondza commented Apr 24, 2017 •

edited

KostyaSha commented Apr 25, 2017 •

edited

bitwiseman commented Jan 16, 2018 •

edited

recena commented Jan 20, 2018 •

edited