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

Pre-filter the 'Available' plugin manager tab, allow multiple terms #4580

Merged
merged 7 commits into from
Apr 11, 2020

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Mar 14, 2020

There are three similar looking tabs in the plugin manager:

  • Updates shows updates for installed plugins.
  • Available shows available plugins (plugins not currently installed).
  • Installed shows currently installed plugins.

The first and third have a fairly limited number of list entries (typically at most dozens and very low hundreds, respectively), all of which are relevant to administrators in different ways (updates and installed), while the second has a very large number of list entries (almost more than 1000) with a very low average/median relevance to users.

This PR implements a change that hides all elements on the "Available" tab by default, and users are reminded to use the "Search" field to make plugins appear. Search expressions need to be two characters minimum (to support searching for "JS" – sorry R!).

Implementation is fairly hacky, since we're doing all the searching/filtering client-side, we just now hide all rows in the form by default (they're still loaded, but at least not rendered!). Didn't want to bother with a full rewrite here, I lack the mad design skillz to pull off an actually good plugin manager anyway!

Screenshots

Updates tab - unfiltered (largely unchanged)

image

Updates tab - slightly filtered, still supports 1 char filters (largely unchanged)

image

Available tab - unfiltered and empty except for a reminder to use the search field

image

Available tab - unfiltered with 1 character in search field

image

Available tab - filtered with 2 characters in search field

image

Available tab - we now support multiple individual search terms

image

Search vs. Filter

I left the "Filter" term on the UI for Updates and Installed, because there's a list that we filter.

"Search" is the new term for "Available" only, since we're starting out with nothing.

Proposed changelog entries

  • Minor RFE: Don't show all available plugins by default; use search field to find plugins
  • Minor RFE: Allow use of multiple space-separated filter terms in plugin manager

Proposed upgrade guidelines

N/A

Submitter checklist

  • JIRA issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

@daniel-beck daniel-beck added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Mar 14, 2020
function applyFilter() {
var filter = e.value.toLowerCase();
["TR.plugin","TR.plugin-category"].each(function(clz) {
var encountered = {};
Copy link
Member Author

Choose a reason for hiding this comment

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

encountered and TR.plugin-category have been obsolete since #4534.

@@ -10,7 +11,8 @@ time {

#filter-container {
margin: 1em;
text-align: right;
text-align: center;
font-size: 2em;
Copy link
Member

@timja timja Mar 15, 2020

Choose a reason for hiding this comment

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

The search input doesn't seem to fit with the rest of the page to me, did its size need to be increased?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did these changes to make it more noticeable. Now there's no way to use the "Available" tab without it, after all. Also larger font size for the placeholder list item.

Whether these need to apply to other tabs too is more questionable.

Copy link
Member

Choose a reason for hiding this comment

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

alternative design:
daniel-beck#2

Copy link
Member Author

Choose a reason for hiding this comment

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

@fqueiruga PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just go with Tim's design, it's more consistent with the header one. We could also extract it into a searchbox widget in the future.

@timja
Copy link
Member

timja commented Mar 15, 2020

I don't seem to be able to search by the plugins artifact ID any more?
Some plugins can be quite hard to find, and display names vs artifact IDs don't match very well.

So I used to sometimes search by ID instead,

i.e.
workflow-cps-global-lib or git-client,

That seems broken with this change, unless I'm missing something?

@daniel-beck
Copy link
Member Author

That seems broken with this change, unless I'm missing something?

Good point, side effect of searching textContent now. Will fix.

@daniel-beck daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 15, 2020
@fqueiruga
Copy link
Contributor

I don't know if this is a good idea. I think it may pose a problem if just one table behaves differently than the other 2. This kind of inconsistency is confusing and may lead to user errors. Thoughts on this?

Maybe as an alternative we can have the search input with autofocus. Still does not solve the problem but at least the users are prompted to start searching

@fqueiruga
Copy link
Contributor

One thing to keep in mind: users do not read. There's a good chance will instinctively think that there are no available plugins or have a bit of confusion.

@daniel-beck
Copy link
Member Author

One thing to keep in mind: users do not read. There's a good chance will instinctively think that there are no available plugins or have a bit of confusion.

Hence giant fonts for everything and search field in the center rather than on the right.

@fqueiruga
Copy link
Contributor

Still, I find it a bit confusing: empty updates tab means there is nothing to be checked. But the next tab means you need to start typing.

TBH I'd find it more useful if we could have a way to filter by categories, now that we sort of have infrastructure for it.

@daniel-beck
Copy link
Member Author

@fqueiruga Is it the overall design using tabs flowing into the table header row that gives the impression these should all behave alike? Would be help if we did that differently, e.g. as sidepanel items?

@@ -10,7 +11,8 @@ time {

#filter-container {
margin: 1em;
text-align: right;
text-align: center;
font-size: 2em;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just go with Tim's design, it's more consistent with the header one. We could also extract it into a searchbox widget in the future.

@@ -2,6 +2,7 @@
background: white url('search.gif') no-repeat 2px center;
padding-left: 20px;
width: 15em;
font-size: 1em;
Copy link
Contributor

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 case for using the font-size in rems. ems should only be used when the element size depends on where they are (such as hyperlinks or inline monospaced elements)

Recommended font sizes are located on the font.less file

function applyFilter() {
var filter = e.value.toLowerCase().trim();
var filterParts = filter.split(/ +/).filter (word => word.length > 0);
var items = document.getElementsBySelector("TR.plugin");
Copy link
Contributor

Choose a reason for hiding this comment

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

getElementsBySelector is a prototype.js function, and should be avoided. The alternatives are using querySelectorAll (https://caniuse.com/#search=querySelectorAll). If the browser support is not good enough (especially due to Baidu-browser support, then I suggest using getElementsByClassName('plugin') and filter out the elements that are not <tr> within the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unchanged from before, just threw out the other, obsolete selector ☹️

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like I didn't read the diff properly then 👍

@fqueiruga
Copy link
Contributor

I think that it is part of why they feel that they should behave the same, but also because they use rather similar tables.

TBH I think they should still be tabbed, I think it would sort of hide the options if moved into the sidebar

@fqueiruga
Copy link
Contributor

@daniel-beck what is the state of this PR? TBH I feel that showing nothing on the available tab by default is a degradation, maybe we can get some more comments on this (even raise it on a UX SIG meeting.

The multi-term search is really interesting though, and the text copy changes are on point. I believe we should move forward with these. If we can include the filter bar changes that @timja created on daniel-beck#2 it would be even better.

@daniel-beck
Copy link
Member Author

TBH I feel that showing nothing on the available tab by default is a degradation, maybe we can get some more comments on this (even raise it on a UX SIG meeting.

I think there should be a way to work around that feeling. I tried it by making the search more noticeable and adding the placeholder with large text, but clearly that wasn't received well.

  • Rename "Available" to "Find New Plugins" to make it clear what the content is and how to interact with it?
  • Move the search field below the tab bar on that page?
  • Move the page from a tab into the side panel to separate it from the other two tabs that are slightly more alike?

@fqueiruga
Copy link
Contributor

TBH I don't think there are good options here that don't merit of a full page revamp. Adding this much used tab to the sidebar can be confusing, it's sort of hiding it.

One thing I believe would improve the situation tough is to add an autofocus attribute to the search input. That way users will be propmted to start searching right away. Other thing that could help is a filter by categories, now that we have them.

@fqueiruga
Copy link
Contributor

I think there should be a way to work around that feeling. I tried it by making the search more noticeable and adding the placeholder with large text, but clearly that wasn't received well.

TBH I think the reaction was more due to styling than anything else (the search box looked a bit out of place like if that part of the page was zoomed in). Tim's proposal of a search box is noticeable enough and looks like the one on the navbar (helps with UI consistency).

@daniel-beck
Copy link
Member Author

@fqueiruga Could you clarify whether a pre-filtered view is possible with timja's PR, or whether neither that nor this PR currently have are good enough to make pre-filtering work?

Other thing that could help is a filter by categories, now that we have them.

#4591 remains unmerged. I wonder whether we could rely on the "label linking" feature implemented there, and do something like:

Use the search field above, or filter by the following popular labels: [pipeline], [github], [docker]

@timja
Copy link
Member

timja commented Mar 28, 2020

That seems broken with this change, unless I'm missing something?

Good point, side effect of searching textContent now. Will fix.

From what I can tell this isn't fixed yet?

Apart from that I think the autofocus has made it more usable along with the search input being made more prominent

Possibly mentioning the filters above could help

@daniel-beck
Copy link
Member Author

@timja

From what I can tell this isn't fixed yet?

True.

Possibly mentioning the filters above could help

Unclear, could you explain?

@timja
Copy link
Member

timja commented Mar 28, 2020

@timja

Unclear, could you explain?

I just meant this comment,

Use the search field above, or filter by the following popular labels: [pipeline], [github], [docker]

showing that you can filter by labels would be helpful imo

@daniel-beck
Copy link
Member Author

showing that you can filter by labels would be helpful imo

Depends on another unmerged PR to work: #4591

Would see that as followup unless we consider it essential to make this PR work.

@timja
Copy link
Member

timja commented Mar 28, 2020

showing that you can filter by labels would be helpful imo

Depends on another unmerged PR to work: #4591

Would see that as followup unless we consider it essential to make this PR work.

👍

I'm happy with this PR except for the not being able to search by artifactId

Will cause lots of 'false positives' for some search terms, but
can always be cleaned up later through more fine-grained control
what can be searched (e.g. textContent + artifactId)
@daniel-beck daniel-beck removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Apr 1, 2020
@timja timja requested a review from a team April 1, 2020 08:38
@timja
Copy link
Member

timja commented Apr 1, 2020

I've updated the screenshots, any other feedback @jenkinsci/core-pr-reviewers ?

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I think it is a great first step, the list of all plugins was not really usable anyway (and it was a long rendering time contributor). It also enables future additions to the view, e.g.

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 7, 2020
@oleg-nenashev
Copy link
Member

I plan to merge it tomorrow if no negative feedback

@timja
Copy link
Member

timja commented Apr 7, 2020

I think it is a great first step, the list of all plugins was not really usable anyway (and it was a long rendering time contributor). It also enables future additions to the view, e.g.

FTR its still slow as it loads all the plugins but just hides them and then does filtering client side.
Other future improvements could be:

  • adding a search API
  • searching server side rather than client side
  • rework the UI :D

@oleg-nenashev
Copy link
Member

rework the UI :D

I read it as "embed plugins.jenkins.io frontend into Jenkins so that we have focus efforts on a single great UI/UX". Which would be a valiant goal indeed

@oleg-nenashev
Copy link
Member

@daniel-beck @timja I resolved the merge conflict with #4580 . Would appreciate a second look before I merge it

@timja
Copy link
Member

timja commented Apr 10, 2020

[ERROR] Failed to execute goal org.jenkins-ci.tools:maven-hpi-plugin:3.11:generate-taglib-interface (default) on project jenkins-core: Failed to generate taglib type interface: Failed to parse /Users/timja/projects/jenkinsci/jenkins/core/src/main/resources/hudson/PluginManager/table.jelly: Error on line 88 of document file:///Users/timja/projects/jenkinsci/jenkins/core/src/main/resources/hudson/PluginManager/table.jelly : Element type "tr" must be followed by either attribute specifications, ">" or "/>". -> [Help 1]

@oleg-nenashev oleg-nenashev added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Apr 10, 2020
@timja
Copy link
Member

timja commented Apr 10, 2020

Tested locally after the last merge fixup ^^, good to go if the build passes

@amuniz
Copy link
Member

amuniz commented May 15, 2020

This is breaking any Acceptance Test trying to install a plugin.

@jsoref
Copy link
Contributor

jsoref commented Aug 6, 2020

Filed https://issues.jenkins-ci.org/browse/JENKINS-63326 about the text...

mamh2021 pushed a commit to mamh-java/jenkins-jenkins that referenced this pull request Oct 14, 2020
fixup '%search' and '%filter' case sensitive in src/main/resources/hudson/PluginManager
in table_xxx.properties and installed_xxx.properties files I found the "filter" startswith a capital letter,
so I choose to the upper case. the "search" is not in any properties files, both upper and lower case are OK,
in order to keep the style same I choose the upper case too.

this PR is fixup for jenkinsci#4580
mamh2021 pushed a commit to mamh-java/jenkins-jenkins that referenced this pull request Jun 12, 2021
fixup '%search' and '%filter' case sensitive in src/main/resources/hudson/PluginManager
in table_xxx.properties and installed_xxx.properties files I found the "filter" startswith a capital letter,
so I choose to the upper case. the "search" is not in any properties files, both upper and lower case are OK,
in order to keep the style same I choose the upper case too.

this PR is fixup for jenkinsci#4580
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 squash-merge-me Unclean or useless commit history, should be merged only with squash-merge web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
7 participants