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

[JENKINS-68282] New design for configure project page #6485

Merged
merged 42 commits into from
Jul 7, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Apr 14, 2022

image

This PR adds a sidebar for quick navigation to the configure project pages, this is intended as a replacement for the tabs on the page.

This brings several advantages:

  • More modern design, fits in with the rest of Jenkins
  • More vertical space, uses horizontal space better
  • More prominent toggle for enabling/disabling a project
  • Supports Symbols for sections
  • Far simpler code for the page

I was going to hold off on opening this for a bit but seeing as there are issue(s) on Jira relating to the page I thought I'd post it now to gather feedback.


This PR doesn't remove the older tabs JS and styling as this is still in use, e.g. on multi branch pipeline projects.


This'll be my last design PR for a while, I'm going to focus on existing PRs and bugs for a good while going forward.

Proposed changelog entries

  • New design for configure project page

Proposed upgrade guidelines

Plugin developers

config-scrollspy.js is now deprecated, see Job/configure.jelly for how to upgrade your configure page if necessary.

Submitter checklist

  • (If applicable) 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) and are in the imperative mood. 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
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

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 accurate, human-readable, and in the imperative mood
  • 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 must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@NotMyFault NotMyFault 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 Apr 15, 2022
@timja timja requested a review from a team April 15, 2022 07:10
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.

I love this and can't wait to apply it to the configure clouds screen which really needs this

ATH appears to be failing

core/src/main/resources/hudson/model/Job/configure.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/form/descriptorList.jelly Outdated Show resolved Hide resolved
Co-authored-by: Tim Jacomb <[email protected]>
@basil basil changed the title New design for configure project page [JENKINS-68282] New design for configure project page Apr 15, 2022
@janfaracik
Copy link
Contributor Author

Opened a draft PR for ATH here jenkinsci/acceptance-test-harness#762

@NotMyFault NotMyFault added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label May 4, 2022
@daniel-beck
Copy link
Member

there wouldn't be an easy way to go back to the Manage Jenkins screen without the sidebar link (no breadcrumb bar link)

#6126 would need to be revived. I got tired of resolving merge conflicts without anyone bothering to review it for months, so closed it.

@daniel-beck
Copy link
Member

Sorry about that. It's caused by one of the (optional) Setup Wizard plugins. I like testing UI stuff by just installing all wizard plugins and seeing how that interacts (in this case, expecting a new top-level section in the job config to appear per previous comments).
Without plugins, the warning doesn't appear, and 2.357 with all plugins also has the warning, so it's unrelated to this PR.

@daniel-beck
Copy link
Member

I've tested it with popular project types and all seems good. Not too sure what a 'dynamic' section is? If it's just the custom sections that the plugins have added to their config pages then all works well.

(Perhaps only before t2d) plugins were able to add new top-level sections (like "Build Environment" etc.) to job config forms. I don't think this was intentional has may have been fixed by the better hierarchical structure of t2d.

@timja timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 5, 2022
@timja
Copy link
Member

timja commented Jul 5, 2022

(I’ll try do a quick POC on another page first to review Daniels concerns)

@timja
Copy link
Member

timja commented Jul 5, 2022

@daniel-beck

POC:
https://github.com/timja/jenkins/pull/new/new-container-configure-system

(single commit view: d21beb2)

it shows yes there is an issue with long labels, but in my instance with quite a lot of plugins it only shows 2 issues, and both could be addressed by shortening the label.

Possibly we would want a way of filtering out some less interesting sections.
There's also an issue with hidden blocks that are behind an f.descriptorRadioList which adds a hidden section.

Full page screenshot

image

Screenshot doesn't demonstrate it very well though as scrolling the page doesn't scroll the sidebar. The only way to move down the sidebar is to get to the bottom of the page.
Again I think this is better done by limiting the number of sections in some way.

any thoughts @daniel-beck / @janfaracik ?

@daniel-beck
Copy link
Member

daniel-beck commented Jul 6, 2022

@timja That's pretty cool, thanks for doing that. If @janfaracik thinks the (non)scrolling issue would be easy to resolve once we get there, that'd be fine with me.

@janfaracik
Copy link
Contributor Author

any thoughts @daniel-beck / @janfaracik ?

@timja That's pretty cool, thanks for doing that. If @janfaracik thinks the (non)scrolling issue would be easy to resolve once we get there, that'd be fine with me.

Thanks for trying that out! I think the ideal solution to the scrolling problem would be to cut down on the amount of sections on the page. I wouldn't personally be opposed to splitting out parts of the page where possible into separate pages, it looks a little unwieldy and uncategorised.

Alternatively we could have the sidebar scroll separately to the rest of the page which shouldn't be too hard to implement if need be.

Cheers!

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 6, 2022
@timja
Copy link
Member

timja commented Jul 6, 2022

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

Thanks!

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

@daniel-beck
Copy link
Member

I wouldn't personally be opposed to splitting out parts of the page where possible into separate pages, it looks a little unwieldy and uncategorised.

🤣 You should have been around before there was /configureSecurity/ (1.425; TBH even I didn't use Jenkins a lot before that existed), /configureTools/ (2.0, initial work in #2011, finished in #2140), and /configureClouds/ (2.205, #4339).

@timja timja merged commit fe41027 into jenkinsci:master Jul 7, 2022
@timja timja deleted the project-config-new-container branch July 7, 2022 11:32
const HEADER_SELECTOR = ".config-table .jenkins-app-bar h2, .config-table > .jenkins-section > .jenkins-section__title";
const DEFAULT_ICON = `<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"><path d="M262.29 192.31a64 64 0 1057.4 57.4 64.13 64.13 0 00-57.4-57.4zM416.39 256a154.34 154.34 0 01-1.53 20.79l45.21 35.46a10.81 10.81 0 012.45 13.75l-42.77 74a10.81 10.81 0 01-13.14 4.59l-44.9-18.08a16.11 16.11 0 00-15.17 1.75A164.48 164.48 0 01325 400.8a15.94 15.94 0 00-8.82 12.14l-6.73 47.89a11.08 11.08 0 01-10.68 9.17h-85.54a11.11 11.11 0 01-10.69-8.87l-6.72-47.82a16.07 16.07 0 00-9-12.22 155.3 155.3 0 01-21.46-12.57 16 16 0 00-15.11-1.71l-44.89 18.07a10.81 10.81 0 01-13.14-4.58l-42.77-74a10.8 10.8 0 012.45-13.75l38.21-30a16.05 16.05 0 006-14.08c-.36-4.17-.58-8.33-.58-12.5s.21-8.27.58-12.35a16 16 0 00-6.07-13.94l-38.19-30A10.81 10.81 0 0149.48 186l42.77-74a10.81 10.81 0 0113.14-4.59l44.9 18.08a16.11 16.11 0 0015.17-1.75A164.48 164.48 0 01187 111.2a15.94 15.94 0 008.82-12.14l6.73-47.89A11.08 11.08 0 01213.23 42h85.54a11.11 11.11 0 0110.69 8.87l6.72 47.82a16.07 16.07 0 009 12.22 155.3 155.3 0 0121.46 12.57 16 16 0 0015.11 1.71l44.89-18.07a10.81 10.81 0 0113.14 4.58l42.77 74a10.8 10.8 0 01-2.45 13.75l-38.21 30a16.05 16.05 0 00-6.05 14.08c.33 4.14.55 8.3.55 12.47z" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="32"/></svg>`;

window.addEventListener("load", function() {
Copy link
Member

Choose a reason for hiding this comment

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

Note that if you are using parent-pom earlier than 4.38, HtmlUnit tests (WebClient.goTo etc.) may fail when run against a jenkins.version of 2.359 or later in PCT:

TypeError: Cannot call method "addEventListener" of null (http://localhost:…/jenkins/static/…/jsbundles/section-to-sidebar-items.js#…)

See jenkins-infra/repository-permissions-updater#2707 for example.

Copy link
Member

Choose a reason for hiding this comment

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

Right yeah there were a couple of plugins in bom affected which we had to get released.

@daniel-beck
Copy link
Member

Caused jenkinsci/depbuilder-plugin#2 (seemingly deliberately, but seems under-documented? Could probably have been done more compatibly?)

@janfaracik
Copy link
Contributor Author

Caused jenkinsci/depbuilder-plugin#2 (seemingly deliberately, but seems under-documented? Could probably have been done more compatibly?)

Ah my bad entirely, I thought we had updated all of the plugins which use that component. I'm happy to fix it and open a PR.

@@ -11,6 +11,7 @@ html {
letter-spacing: -0.016em;
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
scroll-behavior: smooth;
Copy link
Member

Choose a reason for hiding this comment

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

Causes JENKINS-69587.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues 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
Development

Successfully merging this pull request may close these issues.

9 participants