-
-
Notifications
You must be signed in to change notification settings - Fork 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
feat: Add empty state for no old data #10320
base: master
Are you sure you want to change the base?
Conversation
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
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.
UI looks good, jelly just needs a little polish
@timja I've made some changes to the Jelly file , please check now |
change looks good but you haven't done https://github.com/jenkinsci/jenkins/pull/10320/files#r1966735160 |
@timja yup , sorry I missed that earlier , done it now. |
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
@timja made the change now , is it fine? |
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.
LGTM
I don't think that the implementation is complete. While looping over the elements of the table it does the check |
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.
Maybe one can already have a flag when the data is collected whether the table would be empty and the hasExtra will be set.
/label web-ui |
@mawinter69 I've made the desired changes as you suggested |
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
I think it looks good now.
|
@mawinter69 I followed the steps you mentioned and got the correct results : |
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
Any thoughts on "No old data" as the panel text? "No old data was found" to me sounds like it failed to find old data. |
@janfaracik yes , No old data seems a better fit to me too. Shall I change it? |
yes please |
Done |
core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
Outdated
Show resolved
Hide resolved
<j:set var="hasValidData" value="true"/> | ||
</j:if> | ||
<j:if test="${item.value.extra!=null}"> | ||
<j:set var="hasExtra" value="false"/> |
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 this set hasExtra
to true?
I think what @krisstern wanted is that there is an explicit initialization of hasExtra
to false before the loop over the data. Afaik this is not explicitly necessary as an unset variable is evaluated to false, but for clarity this would be better.
Fixes jenkinsci/sig-ux#11.
Added an empty state to Manage Old Data to not display an empty table , instead show No old data was found.
Testing done
Before:


After:
Proposed changelog entries
Proposed changelog category
/label rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@timja @janfaracik
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).