Fix disable delete and drag button on empty name value pair editor

[gijohn]

Fixes: https://jira.coreos.com/browse/ODC-1481

[gijohn]

/kind bug
/assign @spadgett @christianvogt

[gijohn]

/retest

[gijohn]

/retest

[gijohn]

/test e2e-aws

[gijohn]

/assign @joshuawilson @rohitkrai03

[joshuawilson]

@gijohn this is outside of dev console, only Christian can approve it or someone on the admin/core team can.

[joshuawilson]

/unassign rohitkrai03

[joshuawilson]

/lgtm
/approve

[joshuawilson]

@spadgett This appears to work correctly for where we need it but I don't know where core is using it and if there could be any negative impacts. Can you please have someone from your side review.

[spadgett]

Stepping back, I think there's a bit of a divide on handling forms, buttons, and validation between admin console and dev console.

• admin console: leave buttons enabled always, use native browser support for required fields, rely on server for validation
• dev console: disable buttons until form is valid, validate client-side as much as possible before submitting form

I worry that we've lost some consistency in the user experience across console. It becomes more apparent in shared components like this. The existing behavior is more in line with admin console forms and working as intended. The new behavior is more in line with dev console forms. The admin console behavior is largely a holdover from Tectonic console, which took this philosophy. I didn't like the admin console form behavior at first, but having used it for a while, I've come around. It works well!

In 3.x, we performed extensive client-side validation. We ran into a number of bugs around this, either because

1. We had incorrect assumptions / validation logic in the client implementation, or
2. The API changed sometime after the initial console feature was implemented, which caused bugs in later releases.

(2) is hard to avoid no matter how careful you are with the initial implementation. Some of these bugs blocked users from making changes they should be able to make. There was at least one case where we lost data in the route editor because of an API change to routes.

I was happy when we started to let the server do validation, since it avoided having to reimplement complicated server validation logic on the client and seemed more sustainable in the face of future API changes.

I realize this is a small change and a long comment. But I guess my point is the current behavior is more in line with the existing admin console forms. I feel like we should step back and decide what experience we want for forms. Maybe it's OK to have different approaches in different perspectives, but all else being equal, I'd think we'd want consistency.

I'm hesitant to go back to too much client-side field validation. Regular expressions like #2373 scare me :) I'd really prefer to use server dry-run for validation when possible. An easy win might be to align on when we disable buttons, though, for required fields and simple checks.

cc @openshift/team-ux-review

[spadgett]

Can we target 4.3? This is a minor bug, and the current behavior doesn't have any bad effects.

We definitely want to fix #2243 in 4.2, though.

[spadgett]

If we make this change here, we should update list-input.tsx minimally which has the same remove button behavior. There are probably some other similar places.

[joshuawilson]

@christianvogt please take a look.

[christianvogt]

@spadgett I agree with what you're saying about duplicate validation logic and letting the server validate what it can. At the same time, simple validation client side can be helpful as long as it's 100% correct. #2373 is bothersome and will be addressed.

[christianvogt]

@gijohn, it was mentioned that the list-input should also be updated to be consistent with this UX experience change request.

[gijohn]

@christianvogt, I have made the requested changed.

[christianvogt]

/lgtm

[gijohn]

/test e2e-aws-console

[gijohn]

/test e2e-aws-console

[gijohn]

/retest

[christianvogt]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, gijohn, joshuawilson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment