-
Notifications
You must be signed in to change notification settings - Fork 186
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
Store table order and paging size a user select #415
Store table order and paging size a user select #415
Conversation
This is tracked as https://issues.jenkins.io/browse/JENKINS-68677 |
@uhafner thx, I see the issue is open, But there are some parallel fixes. What shall be done to fix this issue also here? |
The last plugin that needs to be adapted is the data-tables-api-plugin. This plugin uses hard coded CSS values (BS5 light style), these CSS files need to be changed to read the Jenkins variables. |
Great, that means, I can ignore it here and maybe (when I find some time) fix it in the |
Jelly changes look like magic - in a good sense, less hardcoding, more functional :) But I can't quickly test it at the moment, the server where we had many resources to lock is currently busy with endurance testing a new product release (so can't restart it). On a side note, could any recent changes elsewhere break the self-tests? Or did they happen to be timing-dependent?
|
Sorry for delay, but I try to split the columns in extra jelly pages, otherwise it is too complex and hard to read. But I am currently stuck with the reset button. It looks so that the code can not be hit. |
Hm I see it is not good idea. Loading jelly pages takes too long. I must find other solution |
There are 2 testcases, their both takes ~50 seconds on my node. In that case the whole test script achieved 180 seconds timeout on slow nodes. I will try to split this testcases to extra scripts to eliminate this scenario. It is very frustrating to make some changes, replays tests and hope that it do not achieved some timeout. I try to eliminate the time consuming there, but both are to complex for me. I hope the split works. Otherwise I will revert test changes here. Has nothing to do with my changes and waste my time. 😪 |
This reverts commit 18d08ad.
fix #411
closes #404
Fixed:
Also we can spare a lot of code.
What is not fixed:
Note: I have try to store also filter text, but it provides more unclear view. There is no indicator in search field (that you has set it it) and you are wondering, why you see only few resources. Therefore I skip this idea.
Testing done
There are few java changes but the changes are hit 100% by code coverage, therefore no new automatic tests. I provide many manual tests and it looks pretty good.
Ordering and paging size are stored for both tables. It works in following cases:
(tested with Chrome Version 107.0.5304.121 (Official Build) (64-bit))
Proposed upgrade guidelines
N/A
Localizations
Note: There is new key 'labels.table.column.percentage' which shall be translated by Crowdin. I tries my best to translate it in all supported languages, but I am not 100 % Sure (@gounthar maybe you can check French here)
Submitter checklist
[ ] There is automated testing or an explanation that explains why this change has no tests.[ ] New public classes, fields, and methods are annotated with@Restricted
or have@since TODO
Javadocs, as appropriate.[ ] New public functions for internal use only are annotated with@NoExternalUse
. In case it is used by non java code theUsed by {@code <panel>.jelly}
Javadocs are annotated.[ ] New deprecations are annotated with@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease the future introduction of Content Security Policy (CSP) directives (see documentation).[ ] For dependency updates, there are links to external changelogs and, if possible, full differentials.[ ] For new APIs and extension points, there is a link to at least one consumer.Maintainer checklist
Before the changes are marked as
ready-for-merge
:[ ] If the change needs additional upgrade steps from users, theupgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).