-
-
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
Add sidebar to plugin manager, increase search bar size #6783
Conversation
This reverts commit 28aaf91.
Do you want something done here? We could try disabling autocomplete if required. —— Can you clarify what is holding back the approval please? |
Available time and competing priorities. I like to review and test nontrivial changes in depth. What I wrote above was initial feedback when running this PR, haven't gotten around to a more extensive test. |
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.
manage/pluginManager/available
's bottom sticker is detached from the background, until you scroll to the bottom of the page and back up:
Kapture.2022-08-23.at.22.34.44.mp4
- I'd be in favor of dropping the search bar suggestions for now, if that's a specific addition of this PR, because this PR targets the sidebar and search bar size. We can reiterate that on a later PR, but I feel minor changes like these end up in delaying PRs longer than needed.
Edit: Disregard the latter, the same behavior is present on recent weeklys.
I haven't managed to reproduce this (on chrome) Can you reliably reproduce it? and are there any specific steps |
Dashboard -> Manage Jenkins -> Manage Plugins -> Available plugins on a clean installation let me reproduce it with Chrome 104. I can't reproduce it reliable it with Safari or Firefox :/ |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
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.
Looks fancy 🎉
I'm fine with disregarding my previous concern, given input suggestions aren't introduced here and do already exist on the past weeklys.
/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! |
Wow, this looks great! I am very pleased with the result and I think this is a huge step forward. I only have one piece of optional non-blocking feedback for your consideration. When I click on "Download progress" on the sidebar, I get to a page with "Download progress" as its heading. When I click on "Advanced settings" on the sidebar, I get to a page with "Advanced settings" as its heading. However, when I click on "Updates", "Available Plugins", or "Installed Plugins" on the sidebar, I get to a page with "Plugins" as its heading. To me, this is unexpected because I would expect the heading of the page to match the navigation link. To me, this feels like it is possibly a relic of the older implementation (where these three pieces of functionality were on a single page with three tabs) that was unintentionally carried through into the redesigned version. But whether or not it was intentional, it seems inconsistent and therefore a potential source of confusion to users. My vote would be to make the headings consistent with the navigation links in all cases. If I am wrong about something here, please feel free to correct me. |
I think that will be mostly addressed when #6783 (comment) is sorted. The title will move into the sidebar and the plugin manager pages themselves won't have a main title. Basically the 3 tabs are being considered as part of 1 page almost. If no objections I plan to merge this tomorrow towards the next weekly. |
.task-icon-badge { | ||
position: relative; | ||
color: currentColor; | ||
letter-spacing: 0; | ||
z-index: 0; | ||
margin-left: auto; | ||
min-width: 24px; | ||
text-align: center; | ||
padding: 0 8px; | ||
line-height: 0.75rem; | ||
font-size: 0.75rem; | ||
|
||
&::before { | ||
content: ""; | ||
position: absolute; | ||
inset: -6px 0; | ||
background: var(--item-background--hover); | ||
border-radius: 100px; | ||
z-index: -1; | ||
} | ||
} |
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.
Unused in this PR, only used in #7084.
.task-link-text { | ||
display: contents; | ||
} | ||
|
||
.task-link-text { | ||
display: contents; | ||
} |
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.
Improperly resolved conflict, or what is this for?
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.
You're correct 👍 I've removed it as part of #7084
This is a small-ish PR to add a sidebar to the plugin manager, as well as to increase the size and prominence of the search bar.
What's changed
Before

After

Screen.Recording.2022-07-05.at.17.32.21.mov
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgrade@Restricted
or have@since TODO
Javadoc, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
if applicable.Desired reviewers
@jenkinsci/sig-ux
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are accurate, human-readable, and in the imperative moodupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).