-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Modernise the table design #5851
Conversation
I couldn't see a need for the custom tag lib so I changed it to use the one the other icons are using, feel free to revert or we can discuss :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new design is fantastic. It gives Jenkins a new and impressive look and feel. Thank you very much for this incredible effort! 🚀
I have some minor non-blocking comments though:
- How can other plugins benefit from the new design without much refactoring? Can we improve the adoption by simply changing existing classes rather than adding new ones?
- Would it make sense to describe the required steps for plugins (to behave correctly) somewhere in the developer documentation? This would help to get a consistent user interface for the whole area of Jenkins. (This can be done in a followup PR)
- How can we combine these changes with the existing Bootstrap and Datatables components so that everything is using the same styling? (Example: https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/lastBuild/cpd)
<j:arg value="${name}"/> | ||
<j:arg value="${title}"/> | ||
</j:invokeStatic> | ||
<j:out value="${icon}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks somehow strange that we need to invoke sever code to render an image. I'm using a SVG sprite to render Font Awesome icons.
How can I add an additional class
to the icon? E.g., to change color or size?
I used that approach in the font-awesome plugin:
https://github.com/jenkinsci/font-awesome-api-plugin/blob/master/src/main/resources/font-awesome/svg-icon.jelly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's explained in the description, it's to avoid having to modify them into sprites.
E.g., to change color or size?
I assume for now it would be wrap it in a span or a div and style that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! The parent element would manage the styling for the icon, although modifier classes should continue to work.
@@ -57,6 +69,50 @@ public static void initPageVariables(JellyContext context) { | |||
context.setVariable("icons", icons); | |||
} | |||
|
|||
private String prependTitleIfRequired(String icon, String title) { | |||
if (StringUtils.isNotBlank(title)) { | |||
return "<span class=\"jenkins-visually-hidden\">" + title + "</span>" + icon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the <title>
tag for the Font Awesome icons: https://github.com/jenkinsci/font-awesome-api-plugin/blob/master/src/main/resources/font-awesome/svg-icon.jelly
@@ -26,6 +26,6 @@ THE SOFTWARE. | |||
<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" xmlns:i="jelly:fmt"> | |||
<td style="${indenter.getCss(job)}"> | |||
<!-- TODO consider using ${rootURL}/${job.url} instead (also in other column.jelly) --> | |||
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link inside'> <l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/></a> | |||
<a href="${jobBaseUrl}${job.shortUrl}" class='jenkins-table__link model-link inside'> <l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we reuse the existing classes? Otherwise we need to change every plugin to get it adapted to the new style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't existing classes for a link inside of a table afaik, and styling .jenkins-table a
could have unintended consequences where a developer wants a link that doesn't look like .jenkins-table__link
.
@@ -29,7 +29,7 @@ THE SOFTWARE. | |||
<j:choose> | |||
<j:when test="${lstBuild!=null}"> | |||
${lstBuild.timestampString} | |||
- <a href="${jobBaseUrl}${job.shortUrl}lastStableBuild/" class="model-link inside">${lstBuild.displayName}</a> | |||
<a href="${jobBaseUrl}${job.shortUrl}lastStableBuild/" class="jenkins-table__link jenkins-table__badge model-link inside">${lstBuild.displayName}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not break, yes. But it uses the old styles. Wouldn't it be better to automatically get the new design for all?
</j:if> | ||
<j:if test="${!empty(healthReports)}"> | ||
<div class="jenkins-tooltip healthReportDetails"> | ||
<table class="bigtable stripped-odd"> | ||
<table class="jenkins-table"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here: we need to restyle all existing plugins. Isn't there a better alternative without changing the plugins by substituting the existing classes?
// Colors | ||
--green: #1c9146; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will bring up that topic in a separate mail (see UX meeting) but wouldn't it make more sense to use some kind of semantics color names? Why is this green different from the success green?
-webkit-border-vertical-spacing: 0; | ||
} | ||
|
||
& > thead { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we then fix those tables?
} | ||
} | ||
|
||
&__cell__button-wrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a non UI developer I also find this new style syntax complex to understand. I have no idea how I can apply those new styles for the Bootstrap or DataTables plugins 🤷
@@ -0,0 +1 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" class="ionicon" viewBox="0 0 512 512"><title>Ellipse</title><circle cx="256" cy="256" r="192" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="32"/></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we use the other icons? So we need to manually add them one by one? Wouldn't it make more sense to include all (like in the Font Awesome plugin)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd involve copying the icons from ionicons to this folder, wanted to avoid that in this PR as it'd balloon this PR up in size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icons are currently part of resources folder (and not below the webapp folder). How can we replace icons in actions with these new ionicon icons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't yet, it wasn't part of this feature.
likely the l:icon tag needs to be able to detect its an ionicon and load it, possibly another flag or other way
@@ -29,7 +29,7 @@ THE SOFTWARE. | |||
<j:choose> | |||
<j:when test="${lstBuild!=null}"> | |||
${lstBuild.timestampString} | |||
- <a href="${jobBaseUrl}${job.shortUrl}lastStableBuild/" class="model-link inside">${lstBuild.displayName}</a> | |||
<a href="${jobBaseUrl}${job.shortUrl}lastStableBuild/" class="jenkins-table__link jenkins-table__badge model-link inside">${lstBuild.displayName}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through here I can't see any blockers, some questions that would be nice to have answers to, but we can iterate all of it.
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.
Thanks!
Hey developers. The table layout changes may be fine, I don't want to discuss that. But the font changes are simply horrible. All header text is now automatically converted to uppercase and uses a huge font spacing (I assume that is the term for empty space between the characters). The views table header suddenly needs much more space (it uses 2 lines instead of 1 for my unchanged views), and the labels in the header are much harder to read now. EDIT: I've meanwhile set this in my installation to make headers readable again:
|
I'm not sure if you are looking into Jira as well: I created a bug report regarding the health-report tooltip styling: |
As @Bananeweizen mentioned, the automatic capitalization is a problem. We have some views that start with capital letters and some views that start with lower-case letters. We got used to the latter being sorted after the former but now it is indistinguishable why the alphabet starts twice, like Before: Apple Pear avocado peach After: APPLE PEAR AVODACO PEACH The issue appeared several times but never really got resolved so please, could you refrain from auto-capitalizing everything? The case does matter for use. https://issues.jenkins.io/browse/JENKINS-8860 |
Can you please file an issue in Jira so we can track this problem? |
Is there any way to make the new UI more compact other than overriding with a custom css? IMHO The new "pills" aproach added too much vertical spacing for an operational UI.
|
Can you create (at least one if not more) issue(s) please with more details, screenshots would be helpful, I understand some of what you mean but not all. You'll need custom css or an improvement PR here. |
Sure 👍 |
Fixed here :) |
&__link, .sortheader { | ||
display: inline-flex; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inline-flex
may have caused JENKINS-68205.
vertical-align: middle; | ||
padding: var(--table-padding) 0 var(--table-padding) var(--table-padding); | ||
font-weight: 500; | ||
height: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to any table using a height of 50 px per row, while you probably wanted this only for the table listing the Jenkins jobs. For instance the test results table looks horrible with this huge space now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please file an appropriate issue labeled as regression on Jira (https://issues.jenkins.io/secure/Dashboard.jspa) including steps to reproduce and screenshots from the old and current behavior.
Causes JENKINS-68303. |
Causes JENKINS-68402. |
@@ -57,6 +66,43 @@ public static void initPageVariables(JellyContext context) { | |||
context.setVariable("icons", icons); | |||
} | |||
|
|||
private static String prependTitleIfRequired(String icon, String title) { | |||
if (StringUtils.isNotBlank(title)) { | |||
return "<span class=\"jenkins-visually-hidden\">" + title + "</span>" + icon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caused SECURITY-2761
📸 View screenshots
Dashboard
Old
New
People
Old
New
Build history
Old
New
What's changed and what's the reasoning behind it
This MR updates the design of the table component in Jenkins, offering a more modern appearance that's easier to use and is easier to work with.
This MR also adds an
ionicon
tag - Ionicons is a wonderful icon library that I think would look great in Jenkins - it's open source, MIT licensed and super easy to integrate with.The eventual plan if this MR is approved/merged would be to update the rest of the tables in Jenkins to follow this format, and then to update instances of icons to use Ionicons, eventually deprecating older styles/icon sources.
Test this pull request
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).