-
-
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
Revamp build history search bar #5692
Conversation
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 manually tested but looks really nice :)
@@ -2292,7 +2291,7 @@ function updateBuildHistory(ajaxUrl,nBuild) { | |||
} | |||
|
|||
function loadPage(params, focusOnSearch) { | |||
var searchString = pageSearchInput.value; | |||
const searchString = pageSearchInput.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.
Cannot use const
on the hudson-behavior.js
file as it's not processed through webpack. These should be var
s
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.
Done.
Looks good. I do have some comments about some issues that will arise. I'd also suggest using, if possible, the |
@janfaracik Looks like this one is almost ready to go. WDYT about the comments by @fqueiruga ? |
They sound sensible to me 👍 I'll be making some changes, I'll post another comment when it should be good for another look over. |
…into separate file
I've updated the PR based on the reviews - thanks everybody by the way :) I've added debounce so server calls are limited, there's a short delay after typing before calling the API so it shouldn't swamp the API. I've also added a small animation to indicate that it's loading and I've also a small notice when there are no builds. Build.history.movReviews are welcome, I've extracted the JS into its own file now which makes things cleaner and easier to maintain - the file could definitely be cleaner though so would be great for people to try out the PR/suggest improvements. Cheers |
Wow I just tried out the old behaviour and that is not a nice UX, never used this before tbh |
Spacing appears to be off for me too, it doesn't appear like in the video above. The doesn't look the best in dark theme but looks like there's variables there that can be used to tweak that |
Darn Safari handles padding/line height differently for search inputs :doh: Should be all fixed now :) The clear button also now changes the cursor on hover. |
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 focus appears to be quite strong (and different from the rest of the application). Not that I like the focus state in the rest of the application.
I think in this case it looks fine, although if it was rolled out more widely it would probably be too 'much' when there's a number of inputs together, but I could be wrong.
My previous feedback was addressed and this is a lot better than the previous search bar
conflicts |
@fqueiruga any chance you could take another look? |
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 think this is a great PR. I'm still a bit concious about the use of px
but that's not a blocker.
Cheers @janfaracik , this is really nice.
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
Suggested Edit:
|
Possibly caused https://issues.jenkins.io/browse/JENKINS-66753 |
Investigating this now... Update: Fix up for review now - #5760 |
Proposed changelog entries
Video
Screen.Recording.2021-08-25.at.20.58.33.mov
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeMaintainer 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).