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 autocomplete #9453

Merged
merged 7 commits into from
Aug 14, 2024

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Jul 11, 2024

Further reduces usage of YUI by switching autocomplete fields to the dropdown component.

Screenshots Before

image

After

image

Testing done

Manual testing with the job trigger for freestyle jobs (see screenshot)

No automated tests -- this could be only covered by UI tests and I'm not sure those would be worth adding/maintaining.

Proposed changelog entries

  • Use dropdown component for autocomplete fields (instead of YUI framework).

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@zbynek zbynek mentioned this pull request Jul 13, 2024
14 tasks
@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 labels Jul 13, 2024
@timja timja requested review from a team July 13, 2024 11:23
@timja timja added the needs-security-review Awaiting review by a security team member label Jul 13, 2024
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.

LGTM worked in testing on a freestyle project and on the design library

@janfaracik
Copy link
Contributor

Looks good to me - any thoughts on making the dropdown full width?

@zbynek
Copy link
Contributor Author

zbynek commented Jul 15, 2024

any thoughts on making the dropdown full width?

I don't have a preference. @timja @NotMyFault WDYT?

image

@timja
Copy link
Member

timja commented Jul 16, 2024

any thoughts on making the dropdown full width?

I don't have a preference. @timja @NotMyFault WDYT?

image

is that after or before?

@zbynek
Copy link
Contributor Author

zbynek commented Jul 16, 2024

The last screenshot is potential after (not implemented), screenshot of the current implementation is in the PR description.

@timja
Copy link
Member

timja commented Jul 16, 2024

Current one is cut-off though so can't really see the impact of the change unless I'm misunderstanding

@zbynek
Copy link
Contributor Author

zbynek commented Jul 16, 2024

Without cutoff:
image
The upside is that with more suggestions the narrow popup covers fewer UI elements, the downside is that validation of partial input may be visible below (as shown in the screenshot in description).

@timja
Copy link
Member

timja commented Jul 16, 2024

Ah I see, I think full width is better but no strong preference

@zbynek
Copy link
Contributor Author

zbynek commented Jul 16, 2024

Full width pushed, this is now ready from my side, thanks for the feedback.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

This unfavorably interacts with browser built-in autocompletion based on form history.

new.mov
old.mov

Additionally, it seems error-prone and bad UX for the popup to close when you click inside the form field, rather than staying open.

@zbynek
Copy link
Contributor Author

zbynek commented Jul 29, 2024

@daniel-beck thanks for pointing those out, both fixed in the last commit

@daniel-beck daniel-beck self-requested a review July 29, 2024 19:03
Co-authored-by: Tim Jacomb <[email protected]>
@mawinter69
Copy link
Contributor

I created JENKINS-73562 to track the removal of YUI from autocomplete

@zbynek
Copy link
Contributor Author

zbynek commented Aug 10, 2024

@daniel-beck will you have time to check if my last changes address the issues you mentioned?

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

The current state addresses both previous issues I've identified.

@timja
Copy link
Member

timja commented Aug 13, 2024

@daniel-beck does this still need a security review?

@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
@daniel-beck
Copy link
Member

@timja Thanks, forgot.

@timja
Copy link
Member

timja commented Aug 13, 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 Aug 13, 2024
@NotMyFault NotMyFault merged commit 9570746 into jenkinsci:master Aug 14, 2024
16 checks passed
@zbynek zbynek deleted the autocomplete branch August 15, 2024 00:49
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.

6 participants