Skip to content
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 icons to Command Palette #10049

Merged
merged 23 commits into from
Dec 16, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Dec 11, 2024

image image

image

Medium-ish PR to add icons to the newly introduced Command Palette. This is one of the first refinements to search, with the intention of making search more useful and visual for users. Actions can override getSearchIcon() to add an icon, or if they implement IconSpec getSearchIcon() will use that by default.

Big thanks to @timja for help on this :)

Testing done

  • Icons show in the Command Palette
  • Bitmaps also show (e.g. user avatars)

Proposed changelog entries

  • Add icons to Command Palette

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@janfaracik janfaracik added the web-ui The PR includes WebUI changes which may need special expertise label Dec 11, 2024
@timja timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Dec 11, 2024
@timja
Copy link
Member

timja commented Dec 11, 2024

@janfaracik add the rfe changelog for a minor enhancement to changelog.

web-ui is just for filtering for people interested it doesn't do anything itself

@jtnord
Copy link
Member

jtnord commented Dec 11, 2024

I guess this comment is relevant here.
Why is IconSpec javadoc not being updated if it is no longer just for Actions/ManagementLinks?

@timja
Copy link
Member

timja commented Dec 11, 2024

I guess this comment is relevant here. Why is IconSpec javadoc not being updated if it is no longer just for Actions/ManagementLinks?

done

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 12, 2024
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@janfaracik janfaracik marked this pull request as ready for review December 13, 2024 09:43
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 13, 2024
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, (tested locally)

Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me too!

@timja
Copy link
Member

timja commented Dec 13, 2024

Looks like a test is failing consistently on this @janfaracik ?

@krisstern
Copy link
Member

Yeah, I see the following in the logs:

19:31:19  [INFO] Results:
19:31:19  [INFO] 
19:31:19  [ERROR] Failures: 
19:31:19  [ERROR]   AbstractProjectTest.autoCompleteUpstreamProjects:630->testAutoCompleteResponse:669 
19:31:19  Expected: is <true>
19:31:19       but: was <false>
19:31:19  [INFO] 
19:31:19  [ERROR] Tests run: 2212, Failures: 1, Errors: 0, Skipped: 55
19:31:19  [INFO] 
19:31:19  [ERROR] There are test failures.
19:31:19  
19:31:19  See /home/jenkins/agent/workspace/Core_jenkins_PR-10049/test/target/surefire-reports for the individual test results.

@timja
Copy link
Member

timja commented Dec 13, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 13, 2024
@krisstern
Copy link
Member

It looks like we also have a NullPointerException according to the logs at https://github.com/jenkinsci/jenkins/pull/10049/checks?check_run_id=34386281462:

java.lang.NullPointerException: Cannot invoke "org.htmlunit.html.DomElement.getHtmlElementDescendants()" because "doc" is null
	at org.htmlunit.html.HtmlPage.executeDeferredScriptsIfNeeded(HtmlPage.java:1466)
	at org.htmlunit.html.HtmlPage.initialize(HtmlPage.java:265)
	at org.htmlunit.WebClient.loadWebResponseInto(WebClient.java:701)
	at org.htmlunit.WebClient.loadWebResponseInto(WebClient.java:575)
	at org.htmlunit.WebClient.getPage(WebClient.java:493)
	at org.htmlunit.WebClient.getPage(WebClient.java:402)
	at org.htmlunit.WebClient.getPage(WebClient.java:538)
	at org.htmlunit.WebClient.getPage(WebClient.java:520)
	at org.jvnet.hudson.test.JenkinsRule$WebClient.getPage(JenkinsRule.java:2735)
	at org.jvnet.hudson.test.JenkinsRule.after(JenkinsRule.java:518)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:676)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:840)

Also one seen at https://ci.jenkins.io/job/Core/job/jenkins/job/PR-10049/7/:

02:14:48  [ERROR]   FileParameterValueTest.fileParameter_canStillUse_internalHierarchy » NullPointer Cannot invoke "org.htmlunit.html.DomElement.getHtmlElementDescendants()" because "doc" is null
02:14:48  [INFO] 
02:14:48  [ERROR] Tests run: 2212, Failures: 0, Errors: 1, Skipped: 55
02:14:48  [INFO] 
02:14:48  [ERROR] 

@timja timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 13, 2024
for (SuggestedItem item : getSuggestions(req, query))
r.suggestions.add(new Item(item.getPath(), item.getUrl()));
for (SuggestedItem curItem : getSuggestions(req, query)) {
String iconName = curItem.item.getSearchIcon();
Copy link
Member

@timja timja Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nonblocking as pre-existing, but can potentially be enhanced in a followup)

One pre-existing issue is that items with a display name are found twice, (even when the actual name is nothing alike)

image

But the display name version isn't an instanceof Item, which means it just gets the default icon.

Ideally it wouldn't report twice or at least it would provide the right item.

See pre-existing screenshot from ci.jenkins.io:
image

@timja
Copy link
Member

timja commented Dec 15, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 15, 2024
@timja timja merged commit 87e9465 into jenkinsci:master Dec 16, 2024
16 checks passed
@timja timja deleted the add-icons-to-command-palette branch December 16, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants