Fixes #38853 - Change import of AutocompleteInput

[Lukshio]

Derived from: #89
Depends on: theforeman/foreman#10734

[Lukshio]

Tests are failing because required component is not merged yet

[ekohl]

Typo: Certificate, not Certification

[Lukshio]

Thanks! I didn't notice, I just copied the old values :)

Fixed in original PR: #89

[pondrejk]

Checking with stream + packit with this PR and the Foreman one.
There is an e2e UI test for webhooks in robottelo, I'll be running it against this, will report with results.
For now, just some comments (I'm on firefox):

I think the label spacing could confuse people to think that the labels belong to the fields above not below if you catch my drift. Also the spacing seems to be inconsistent, here Target URL has more space above it than other labels

I'm confused by the behavior of the validation icons for all the inputs, you can get red when searching non existent on first try:

then do a successful search and try again with non-matched string, you get green

Also empty field doesn't get icon on a first visit, but try searching and then delete the text and the icon remains for the empty field

Also not sure if the icon makes sense for fields like name? Is there really any real-time validation going on? The icon also gets displayed for the HTTP method picker where you really can't make a wrong pick, can you? Well, maybe if typing some nonsense in, but then the icon is still green. Here I'd say a basic dropdown without search would do.

I see there is an url validation in place, but again, the warning is not cleaned up when text is deleted

So maybe we could get rid of these validation icons altogether in places where no real validation is in place, if that's configurable in the component. Or maybe we could add those validations if thats possible.

On a more general note, if an item selection is mandatory and it is to be selected from a defined set of items it is imho a design flaw to allow users to leave the field empty or type nonsense to it -- no blaming you, I know this pattern existed in the old UI, but maybe this is an opportunity to make it less confusing. Searching through patternfy for best practices, I guess we could turn these fields into single select filters with search capability if there is a longer list of options, something like

taken from https://www.patternfly.org/patterns/filters/design-guidelines/#filter-types

[pondrejk]

made some comments on UI, downstream test result tba

[pondrejk]

I have run pytest tests/foreman/ui/test_webhook.py -k test_positive_end_to_end from robottelo, getting the selenium locator error, the test passes on latest stream, so the automation adjustment is needed to react to these changes. I'll go on to do it once this PR is finalized. I think we should add "breaks robottelo" tag, I don't have rights to do that

[Lukshio]

Hi, thank you for the review @pondrejk

Spacing between inputs fixed by var(--pf-v5-c-form--GridGap)
The validation inputs:

• First try is now not validated as invalid
• searching again keeps green because the selected option rests selected even thought it does not match any option. The component is from https://www.patternfly.org/components/menus/select/react-templates/typeahead/
• Name and Url are validated because the field is required. That is also why the warning is present if input is deleted.

[pondrejk]

thanks, I know packit was down, looking at the rpm build task it is queued for 2 days which is suspicious :) I'll try to rebuild with the comment but I suspect I might not have rights to do it

[pondrejk]

/packit build

[packit-as-a-service]

Account pondrejk has no write access nor is author of PR!

[adamruzicka]

/packit build

[pondrejk]

@Lukshio thanks for the updates, couple of points came to mind:

• some placeholder would be nice to make people aware that the field is actually a search field, old UI had "Start typing to search", I'd say it should stay

• when I type something invalid to the search, then change focus from the field, the field is cleared out (or switched to the previous selection if it's not the initial try), which is I guess what we want. But then when I roll down the menu I still see the message related to the previous search, not the valid options for selection:

Interestingly, the patternfly typeahead you shared behaves the same, which is I think is a bug on their side. Though when I look at the pf5 version of that template, the behavior is correct there https://v5-archive.patternfly.org/components/menus/select/react-templates/typeahead/ -- so I think we should aim for this

• I still think it is wrong to show validation symbols for fields that are not validated or not mandatory, for example the HTTP Headers fields wants a specific structure, we don't validate it, but we make it seem like we do

Otherwise it looks nice, so I'll go on to adjust ds automation for this, nice work

[pondrejk]

working on downstream tests, I wonder if we can create some unique ids for the fields, in latest version I see id=typeahead-select-input 3 times for the first tab

[Lukshio]

@pondrejk Sure, I will add the unique IDs, fix the validations, and add placeholders. I will make all the changes after approve from @adamruzicka in parent PR and then rebase it here.

[pondrejk]

Hi @Lukshio thanks for the improved UI, I have no further comments about the looks, but I see some functional issues with the searchable dropdowns -- when I check the dropdown after selecting some value or the value is preselected (for example http method) I can only pick the selected option

Sometimes (maybe on the first pass) I can see all options, but just clicking outside of the box reduces the list back to the selected one.

So the only way to change value is by clearing the field and typing the new value, but even after clicking the new value in the list, the selection moves back to the original value.

I got some surprising full page refreshes while working with the form, though I can't reproduce them regularly, not sure if it could be related to your changes, mentioning it just in case, maybe it'll sound familar

Confirming the unique IDs are there, thank you for that

[adamruzicka]

Needs a rebase now

[Lukshio]

@adamruzicka rebased, but it needs to be merged after this one theforeman/foreman#10734

[adamruzicka]

Now this is a reasonably sized PR :)

Could you please bump required foreman version in engine.rb?

[Lukshio]

Now this is a reasonably sized PR :)

Could you please bump required foreman version in engine.rb?

Updated to 3.17

[adamruzicka]

Updated to 3.17

That's not enough, is it? If the foreman part was merged a couple of days ago into what will eventually be 3.18?

[Lukshio]

Updated to 3.17

That's not enough, is it? If the foreman part was merged a couple of days ago into what will eventually be 3.18?

That's good point, edited

[adamruzicka]

LGTM

[adamruzicka]

Thank you @Lukshio !