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

Replace hetero-list YUI button and menu with new style button and tippy.js menu #8340

Merged
merged 15 commits into from
Aug 21, 2023

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Aug 6, 2023

use new button style
replace the YUI menu with a tippy menu

Following plugins use the hetero-list-container and input button at some places without using the hetero-list.jelly. The input button is replaced by a jenkins button in javascript.

  • dingtalk
  • envinject
  • branch-api
  • scm-api
  • multiple-scms
  • pipeline-model-definition

Some Notes

  • The tippy menu is inserted directly after the button for easier navigation for keyboard users.
  • When using the "Tab" key to go through the page, once this selects a menu entry, this will become the selected entry. When you now use the "ArrowUp" or "ArrowDown" keys, focus will follow the selected entry.
  • menualign attribute is now ignored (not sure what it was doing at all)

Before:
image
image

After:
image
image

Testing done

Freestyle jobs

  • Create freestyle project and goto configure
  • Add post build step -> Corresponding form it inserted
  • Add another post build step (previously inserted step is disabled)

Pipeline jpbs

  • create a pipeline job
  • go to "Pipeline Syntax" -> "Declarative Directive Generator"
  • Select "options: Options"
  • Click "Add" -> "Timestamps" -> Corresponding form is inserted.
  • Click "Generate Declarative Directive" -> Snippet is properly generated
  • Select "tools: Tools" (This is not using the hetero-list.jelly but directly refers to relevant classes)
  • New button/menu is used
  • Click "Add" -> "Git"
  • Click "Generate Declarative Directive" -> Snippet is properly generated

Proposed changelog entries

  • Replace hetero list YUI button and menu with new style button and tippy menu.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@mawinter69 mawinter69 marked this pull request as ready for review August 7, 2023 10:58
@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Aug 12, 2023
@timja timja requested a review from a team August 12, 2023 21:18
timja added a commit to jenkinsci/acceptance-test-harness that referenced this pull request Aug 12, 2023
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Aug 12, 2023
@timja timja requested a review from a team August 12, 2023 21:20
@timja timja added ath-fail The acceptance-test-harness suite needs a forward-compatible change ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed ath-fail The acceptance-test-harness suite needs a forward-compatible change labels Aug 13, 2023
Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Looks OK security wise

@yaroslavafenkin yaroslavafenkin removed the needs-security-review Awaiting review by a security team member label Aug 14, 2023
@yaroslavafenkin yaroslavafenkin added the security-approved @jenkinsci/core-security-review reviewed this PR for security issues label Aug 14, 2023
@janfaracik
Copy link
Contributor

Looking good! I was working on something similar a while back but didn't get as far with it.

Few things -

It'd be nicer if the filter bar was more integrated with the menu, it's looking quite heavy at the moment.
image

I haven't had a chance to run it yet but from the screenshots it looks like the menus width will change when you filter, it'd be nicer if the menu width was fixed so it won't change when the items change.

I think changing the item height to be smaller makes the usability harder, it'd be harder to click and for users will accessibility needs.

@mawinter69
Copy link
Contributor Author

Looking good! I was working on something similar a while back but didn't get as far with it.

Few things -

It'd be nicer if the filter bar was more integrated with the menu, it's looking quite heavy at the moment.

done

I haven't had a chance to run it yet but from the screenshots it looks like the menus width will change when you filter, it'd be nicer if the menu width was fixed so it won't change when the items change.

The Width changes with applied filters, but this was also the case before. Seems it is not possible with pure CSS to prevent this.

I think changing the item height to be smaller makes the usability harder, it'd be harder to click and for users will accessibility needs.

My intention was as there are no icons involved and the number of entries easily get over 20 (with flexible publish and any step plugins, I have here 23 entries in the menu) so I wanted it to be more compact. I increased the min-height now to 30px

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.

Really nice, love it

@timja timja requested a review from a team August 19, 2023 16:58
@timja
Copy link
Member

timja commented Aug 20, 2023

/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 Aug 20, 2023
@NotMyFault NotMyFault changed the title refresh hetero-list UI Replace hetero-list YUI button and menu with new style button and tippy.js menu Aug 21, 2023
@NotMyFault NotMyFault merged commit c1a6c6c into jenkinsci:master Aug 21, 2023
16 checks passed
basil added a commit that referenced this pull request Aug 22, 2023
basil added a commit that referenced this pull request Aug 22, 2023
… and tippy.js menu" (#8412)

Revert "Replace hetero-list YUI button and menu with new style button and tippy.js menu (#8340)"

This broke:

- org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScriptTest
- hudson.plugins.textfinder.TextFinderPublisherFreestyleTest#createTextFinderViaWebClient

This reverts commit c1a6c6c.
@basil
Copy link
Member

basil commented Aug 22, 2023

This broke org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScriptTest and hudson.plugins.textfinder.TextFinderPublisherFreestyleTest#createTextFinderViaWebClient

Reverted in #8412

Try again in the next weekly after ensuring that you are not breaking PCT

@basil basil added the skip-changelog Should not be shown in the changelog label Aug 22, 2023
@daniel-beck
Copy link
Member

daniel-beck commented Aug 22, 2023

Try again in the next weekly after ensuring that you are not breaking PCT

If you're this strict about changes causing PCT regressions, while making it the submitters' responsibility, this needs to be documented in https://github.com/jenkinsci/jenkins/blob/master/CONTRIBUTING.md and https://github.com/jenkinsci/jenkins/blob/master/.github/PULL_REQUEST_TEMPLATE.md.

yaroslavafenkin pushed a commit to yaroslavafenkin/jenkins that referenced this pull request Sep 26, 2023
…py.js menu (jenkinsci#8340)

* refresh hetero-list UI

use new button style
replace the YUI menu with a tippy menu

* fix lint errors

* avoid blanks

* prettier

* keep button in div

* fix test

* fix test jenkinsci#2

* convert inputs to buttons for plugins

* prettier

* remove unnecessary style definition

* remove adjunct

* adjust filter

---------

Co-authored-by: Tim Jacomb <[email protected]>
yaroslavafenkin pushed a commit to yaroslavafenkin/jenkins that referenced this pull request Sep 26, 2023
… and tippy.js menu" (jenkinsci#8412)

Revert "Replace hetero-list YUI button and menu with new style button and tippy.js menu (jenkinsci#8340)"

This broke:

- org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScriptTest
- hudson.plugins.textfinder.TextFinderPublisherFreestyleTest#createTextFinderViaWebClient

This reverts commit c1a6c6c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite 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 security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog 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.

7 participants