-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
Add experimental 'Details' widget for builds #10147
base: master
Are you sure you want to change the base?
Add experimental 'Details' widget for builds #10147
Conversation
/** | ||
* Returns true if this detail is applicable to the given Actionable object | ||
*/ | ||
public boolean isApplicable() { |
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.
For Action
this would be the job of the factory. Why create an object just to discard it?
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.
That's fair, I was using isApplicable
to encapsulate show/hide logic of the detail inside of the Detail
, so that its factory didn't need to worry about that.
public static DetailGroup SCM = new DetailGroup(0); | ||
|
||
public static DetailGroup GENERAL = new DetailGroup(Integer.MAX_VALUE); |
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.
Why are these not @Extension(ordinal = …)
?
details.addAll(taf.createFor(object)); | ||
} | ||
|
||
Map<DetailGroup, List<Detail>> orderedMap = new TreeMap<>(Comparator.comparingInt(DetailGroup::getOrder)); |
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 is so weird only because it doesn't use @Extension
.
|
||
for (Detail detail : details) { | ||
if (detail.isApplicable()) { | ||
orderedMap.computeIfAbsent(detail.getGroup(), k -> new ArrayList<>()).add(detail); |
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.
Not extensible if it doesn't handle getOrder
clashes gracefully.
This doesn't look finished enough for a security review, but so far with what's there, there's no concern from my side. @timja Is there specific code that triggered you to label this as needing review that I may have missed? |
Not really wasn't 100% if it was needed, thanks for the feedback |
|
Summary widget is basically 'Uncategorised' from the Manage Jenkins page. I'm thinking for the next core ones:
Plugin ones:
Need to do a bit of analysis to see whats going on the current summary pane and needs to be there / should be removed. e.g. we plan to get rid of all the Git Action data thats displayed there...
I think for the Details card anything else will look quite silly so potentially it will be fine because of the building blocks supplied. Its possible we don't even need this pane to be jelly-able. We started out with it as Jelly but Jan's moved I think most of it to be done in Java now. I think the date format one is the only thing left in Jelly which could potentially be handled in another way if its worth it.
There's an order through code in this. Potentially, we need to review more of what goes on the summary page. |
I don't think that order by code will be sufficient. This will lead to the same problem as we have right now. Before we are developing new widgets we should better spend some time on how this can be configured by users. Like this has been done by e.g. Apple on IOS devices (and now on the desktop as well): first of all, they provided a fixed grid (tiles) and a user configuration of this grid. Users can change the size of the widgets (in terms of tiles), order of widgets, visibility of widgets. Not until they released this concept they added widgets step by step... Otherwise we get the same thing as now: too many widgets compete for the best place in the small visible area. |
Co-authored-by: Tim Jacomb <[email protected]>
Heya. Thanks for the feedback - I do agree, I think end goal is definitely a significant increase in structured customisability from users (and plugin developers) of what the UI can do. Keen to avoid the issues we have now where it's essentially a free-for-all for developers to put what they want where, some hierarchy (without being too restrictive) is definitely needed. What I'd like to do with these experiments is prove out our current thinkings with real world users, e.g. is the 'Details' widget a sensible place for build/SCM info, is it more organised than what we have now, is this the best way to present this information. Right now, though, this is just an experiment. It’s simple to implement and easy to roll back if it doesn’t work out, but I'm hopeful it's a positive addition and users respond well. Looking to blog about it/share it with the community once it's live for feedback/polls. |
Behind the experimental flag,
new-build-page.flag
, this PR adds a new 'Details' widget to build pages. The widget lists information about the build (currently just a small set for this PR), which makes it easy to see at a glance what caused the build, how long it took, and any relevant SCM information.This is just a subset of the final information that could go into this widget, intended to keep the PR small and reviewable.
We have a new extensible API too, which allows plugins, such as the Git Branch Source plugin, to hook in to the Details widget to contribute its own
Detail
items.Downstream PR here jenkinsci/github-branch-source-plugin#826 (Need to move the detail to scm-api will be done soon)
An example of what a more complete implementation of this could look like is seen in the Pipeline Graph View plugin:
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@jenkinsci/sig-ux
Before the changes are marked as
ready-for-merge
:Maintainer checklist