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.

Sign up for GitHub

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

Jump to bottom

Add section titles to Manage Jenkins context menu #4586

Merged
merged 3 commits into from
Apr 12, 2020
Merged

Add section titles to Manage Jenkins context menu #4586

merged 3 commits into from
Apr 12, 2020

Conversation

daniel-beck
Copy link
Member

Alternative to #4583.

Screen shots

Screenshot 2020-03-16 at 13 27 58
Screenshot 2020-03-16 at 13 28 05

Proposed changelog entries

  • Add section headers to context menu of Manage Jenkins

Proposed upgrade guidelines

N/A

Submitter checklist

  • [n/a] 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 alternative This PR is one of several options to address a given issue labels Mar 16, 2020
@daniel-beck daniel-beck mentioned this pull request Mar 16, 2020
Closed
9 tasks
@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 16, 2020
@daniel-beck
Copy link
Member Author

Reworking this to be less of a disgusting hack.

@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 Mar 16, 2020
@daniel-beck @fqueiruga
Larger font size
1f2ad71 
Co-authored-by: Félix Queiruga <fqueiruga@cloudbees.com>
@daniel-beck
Copy link
Member Author

Amended with larger font size (thanks @fqueiruga):

Screenshot

fqueiruga
fqueiruga suggested changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@fqueiruga fqueiruga left a comment

Choose a reason for hiding this comment

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

Looks great! nice, smart solution. Just added a suggestion to rename the CSS class in order to make it more specific for this component.

core/src/main/resources/lib/layout/breadcrumbs.css
@@ -126,3 +126,8 @@
A.model-link.inside, #breadcrumbs A.inside {/* additional 'inside' class allows pre-allocation of the context menu space */
padding-right: 16px;
}

#breadcrumb-menu .header {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#breadcrumb-menu .header {
.breadcrumb-menu__category {

core/src/main/resources/lib/layout/breadcrumbs.js
@@ -203,7 +203,11 @@ var breadcrumbs = (function() {
onComplete:function (x) {
var a = x.responseText.evalJSON().items;
function fillMenuItem(e) {
e.text = makeMenuHtml(e.icon, e.displayName);
if (e.header) {
e.text = makeMenuHtml(e.icon, "<span class='header'>" + e.displayName + "</span>");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
e.text = makeMenuHtml(e.icon, "<span class='header'>" + e.displayName + "</span>");
e.text = makeMenuHtml(e.icon, "<span class='breadcrumb-menu__category'>" + e.displayName + "</span>");

@jtnord
Copy link
Member

jtnord commented Mar 17, 2020 •
edited
Loading

Have we considered how this will negatively effect scrolling...

e.g. it will make situations like the following much more common (and they are annoying)
image

I like the fact the layout matches the actual page more - but I believe the usability could suffer.

fcojfernandez
fcojfernandez approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@fcojfernandez fcojfernandez left a comment

Choose a reason for hiding this comment

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

Although I agree with James' comment, I think the benefits of having those titles are bigger than the effects on the scroll

@varyvol
Copy link

varyvol commented Mar 19, 2020

I like the change, I think it improves the visualization a lot. Happy to approve once you review @fqueiruga suggestions.

oleg-nenashev
oleg-nenashev approved these changes Apr 10, 2020
View reviewed changes
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 am fine with both alternatives, leaving the decision to UX experts.

P.S: We could even have both and select one based on the number of items (e.g. this PR when it is below 12 and #4583 when the number is above). But most likely it is an over-complication

@daniel-beck
Copy link
Member Author

Personally I'd prefer this be merged, and the other being left as a workaround if feedback is overly negative for adding ~5 items.

@oleg-nenashev
Copy link
Member

I plan to merge it tomorrow if no negative feedback

@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 11, 2020
@timja timja merged commit 9bb02ec into jenkinsci:master Apr 12, 2020
@daniel-beck daniel-beck mentioned this pull request May 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@fqueiruga fqueiruga fqueiruga approved these changes

@fcojfernandez fcojfernandez fcojfernandez approved these changes

Assignees
No one assigned
Labels
alternative This PR is one of several options to address a given issue 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@daniel-beck @jtnord @varyvol @oleg-nenashev @fqueiruga @fcojfernandez @timja