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.

Sign up for GitHub

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-65033 Run / Project summary - tables to divs #5351

Closed
wants to merge 3 commits into from
Closed

JENKINS-65033 Run / Project summary - tables to divs #5351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df91053
JENKINS-65033 Run summary - tables to divs
timja Mar 8, 2021
c9bf785
Tweak CauseAction layout to render better (and fix test)
timja Mar 11, 2021
a1b2e97
Extend to Project as well
timja Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/java/hudson/Functions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ public static void initPageVariables(JellyContext context) {
context.setVariable("resURL",rootURL+getResourcePath());
context.setVariable("imagesURL",rootURL+getResourcePath()+"/images");
context.setVariable("divBasedFormLayout", true);
context.setVariable("divBasedRunLayout", true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used in jenkinsci/workflow-job-plugin#180 to adapt to this change without requiring a core bump

context.setVariable("userAgent", currentRequest.getHeader("User-Agent"));
IconSet.initPageVariables(context);
}
Expand Down
8 changes: 5 additions & 3 deletions core/src/main/resources/hudson/model/AbstractBuild/index.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ THE SOFTWARE.
<t:editableDescription permission="${it.UPDATE}" />
</div>

<table style="margin-top: 1em; margin-left:1em;">
<div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how things are done with styling etc now in the modern times. But I think it would be nice if this div had an id for page decorators et.al. if they need to manipulate it somehow. Themes would also have it easier to style it I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really sure why you would need them.

themes should be able to use selectors and if they are finding a need for it they can add them at the time.

<t:artifactList build="${it}" caption="${%Build Artifacts}"
permission="${it.ARTIFACTS}" />

Expand Down Expand Up @@ -107,9 +107,11 @@ THE SOFTWARE.

<!-- give actions a chance to contribute summary item -->
<j:forEach var="a" items="${it.allActions}">
<st:include page="summary.jelly" from="${a}" optional="true" it="${a}" />
<div style="margin-top: 1em;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing quite a lot more of breaking in plugins.
Ex: https://github.com/jenkinsci/build-failure-analyzer-plugin/blob/master/src/main/resources/com/sonyericsson/jenkins/plugins/bfa/model/FailureCauseBuildAction/summary.groovy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure I tested BFA and it rendered fine, can double check later

Copy link
Member Author

@timja timja Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Job page, browser allows the table and it looks the same as before:

image

Run page, browser throws out the table tags, it looks fine except it's using a heading tag which is defined as a block element which moves it down a line

image

We could maybe set an adjacent sibling selector to make headings display inline if after an img tag or svg, any thoughts @fqueiruga ?

Doesn't look like a blocker anyway to me, thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, that looks OK. Hopefully it won't break things rendered after it, though I don't think it will since it closes it's table tags.

<st:include page="summary.jelly" from="${a}" optional="true" it="${a}" />
</div>
</j:forEach>
</table>
</div>

<st:include page="main.jelly" optional="true" />

Expand Down
24 changes: 15 additions & 9 deletions core/src/main/resources/hudson/model/AbstractProject/main.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,21 @@ THE SOFTWARE.

<p:projectActionFloatingBox />

<table style="margin-top: 1em; margin-left:1em;">
<div style="margin-top: 1em; margin-left:1em;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this one too?


<j:forEach var="act" items="${it.prominentActions}">
<j:set var="icon" value="${act.iconClassName != null ? act.iconClassName : act.iconFileName}"/>
<t:summary icon="${icon}" href="${act.urlName}">
${act.displayName}
</t:summary>
<div>
<t:summary icon="${icon}" href="${act.urlName}">
${act.displayName}
</t:summary>
</div>
</j:forEach>
<t:summary icon="folder.png" href="ws/" permission="${it.WORKSPACE}">
${%Workspace}
</t:summary>
<div>
<t:summary icon="folder.png" href="ws/" permission="${it.WORKSPACE}">
${%Workspace}
</t:summary>
</div>

<t:artifactList caption="${%Last Successful Artifacts}"
build="${it.lastSuccessfulBuild}" baseURL="lastSuccessfulBuild/"
Expand All @@ -50,11 +54,13 @@ THE SOFTWARE.
${%Recent Changes}
</t:summary>

</table>
</div>

<!-- merge fragments from the actions -->
<j:forEach var="a" items="${it.allActions}">
<st:include page="jobMain.jelly" it="${a}" optional="true" />
<div>
<st:include page="jobMain.jelly" it="${a}" optional="true" />
</div>
</j:forEach>

<p:upstream-downstream />
Expand Down
15 changes: 9 additions & 6 deletions core/src/main/resources/hudson/model/CauseAction/summary.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ THE SOFTWARE.
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<t:summary icon="orange-square.png">
${%Caused by}
Copy link
Member Author

@timja timja Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed to add something to go next to the icon to make it looks okay,

any better wording or design thoughts?

image

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a more explicit Build started by or Build caused by? Caused by seems fine as well

Not sure whats a good wording for it is.

Personally I'm not a fan of the icon but that's outside the scope of this PR and with the new icons coming in this might be updated at some point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

<ul>
<j:forEach var="entry" items="${it.causeCounts.entrySet()}">
<p>
<st:include page="description.jelly" it="${entry.key}"/>
<j:if test="${entry.value > 1}">
<st:nbsp/>${%Ntimes(entry.value)}
</j:if>
</p>
<li>
<st:include page="description.jelly" it="${entry.key}"/>
<j:if test="${entry.value > 1}">
<st:nbsp/>${%Ntimes(entry.value)}
</j:if>
</li>
</j:forEach>
</ul>
</t:summary>
</j:jelly>
6 changes: 0 additions & 6 deletions core/src/main/resources/lib/hudson/summary.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ THE SOFTWARE.
</st:documentation>
<j:if test="${h.hasPermission(it,attrs.permission)}">
<j:if test="${attrs.icon!=null}">
<tr>
<td>
<!--
Backward compatibility. Ugly but needed for now.

Expand Down Expand Up @@ -93,8 +91,6 @@ THE SOFTWARE.
</j:choose>
</j:otherwise>
</j:choose>
</td>
<td style="vertical-align:middle">
<j:choose>
<j:when test="${attrs.href!=null &amp;&amp; !attrs.iconOnly}">
<a href="${attrs.href}">
Expand All @@ -105,8 +101,6 @@ THE SOFTWARE.
<d:invokeBody />
</j:otherwise>
</j:choose>
</td>
</tr>
</j:if>
</j:if>
</j:jelly>