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

Use standard dropdowns for combobox #9462

Merged
merged 8 commits into from
Oct 21, 2024
Merged

Conversation

timja
Copy link
Member

@timja timja commented Jul 13, 2024

Fixes jenkinsci/design-library-plugin#13

Unfortunately can't delete https://github.com/jenkinsci/jenkins/blob/master/war/src/main/webapp/scripts/combobox.js as editableComboBox still uses it, #9461 removes the last usage in core though.

This improves the usability and accessibility of the control by adding styling when doing keyboard navigation, and makes the code a lot simpler and easier to follow (last one used very old JavaScript design patterns and was hard to understand)

I haven't added support for allowMultipleValues as I couldn't find any usages of it.

Testing done

Tested with the design library example http://localhost:8080/jenkins/design-library/Inputs/

Keyboard navigation, tab and enter on item
Clicking the item
Tabbing away before selecting an item
Editing items and making sure they show up

Unlike autocomplete, combobox shows items immediately so tested some special handling for that

I've also tested with #9461 (which was motivated by this PR)

Proposed changelog entries

  • Modernise combobox component

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

Co-authored-by: Zbynek Konecny <zbynek1729@gmail.com>
@timja timja requested a review from zbynek July 13, 2024 20:05
@timja timja 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 needs-security-review Awaiting review by a security team member labels Jul 13, 2024
@timja timja requested review from a team July 13, 2024 20:06
Comment on lines 95 to 97
if (!selectedItem) {
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

occurs when navigating between potential text left and right without selecting an item (doesn't cause an issue just stacktraces in the console)

e.g. type North, then navigate left on the keyboard

@zbynek
Copy link
Contributor

zbynek commented Jul 13, 2024

Looks like quite a bit of code was copied from #9453, some parts were improved here (e.g. I missed the focusout listener). Maybe it would make sense to land this PR first, then refactor mine to avoid the duplication and make use of the improvements you've done here? Or would you prefer the other order?

@timja
Copy link
Member Author

timja commented Jul 13, 2024

Looks like quite a bit of code was copied from #9453, some parts were improved here (e.g. I missed the focusout listener). Maybe it would make sense to land this PR first, then refactor mine to avoid the duplication and make use of the improvements you've done here? Or would you prefer the other order?

I don't mind either way, I've just noticed that the Dynamic combobox example is broken though so will convert this back to draft and take another look later

@timja timja marked this pull request as draft July 13, 2024 22:34
@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Aug 13, 2024
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 14, 2024
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 13, 2024
@timja timja requested a review from zbynek October 13, 2024 21:27
@timja timja marked this pull request as ready for review October 13, 2024 21:27
@timja
Copy link
Member Author

timja commented Oct 13, 2024

Retested all with the examples on http://localhost:8080/jenkins/design-library/Inputs/

I've refactored out some common code as well.

@timja timja requested a review from janfaracik October 15, 2024 18:47
Copy link
Contributor

@zbynek zbynek left a comment

Choose a reason for hiding this comment

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

Looks good!

@timja
Copy link
Member Author

timja commented Oct 19, 2024

@janfaracik you able to check this please?

@timja
Copy link
Member Author

timja commented Oct 20, 2024

/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 Oct 20, 2024
@timja timja merged commit d7eda6a into jenkinsci:master Oct 21, 2024
16 checks passed
@timja timja deleted the combobox-rework branch October 21, 2024 18:36
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 security-approved @jenkinsci/core-security-review reviewed this PR for security issues 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.

Combo box not accessible
4 participants