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

rework of command forms #411

Merged
merged 5 commits into from
May 15, 2023
Merged

Conversation

west270
Copy link
Contributor

@west270 west270 commented May 10, 2023

Closes #390 #389
Reworked command form to use react hook forms.
The form should have same functionality as previous form and fixes some issues with dynamic commands like with the evaluate command. Also remake request button now works with dynamic commands
The job create path still uses the old command form.

Tracking of issues for follow on tickets: #410

@west270 west270 linked an issue May 10, 2023 that may be closed by this pull request
Copy link
Collaborator

@obr42 obr42 left a comment

Choose a reason for hiding this comment

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

Overall I really like the changes, I think they're much more maintainable. Also really like the more condensed layout. Great work! Pretty much all the comments are minor and/or can be pushed into a follow-on ticket. I'm really excited to see how much we can cut out once we get the job stuff onto this new form

--

Not sure where in the code it is, but I used the echo_list_model and when changing the drop-down for choices got the error below. Has to do with variables assigned to field values starting as explicitly undefined and then being defined upon user input.

MUI: A component is changing the default value state of an uncontrolled Select after being initialized. To suppress this warning opt to use a controlled Select. 

--

Can't use keyboard navigation to change required v optional tabs. When adding to a list in the form, the Add button could use a little more context for screen readers (hidden element or an aria-label that gives context to what is being added)

src/pages/CommandView/CommandForm.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandForm.tsx Show resolved Hide resolved
src/pages/CommandView/CommandView_new.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandView_new.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandView_new.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandView_new.tsx Show resolved Hide resolved
return (
<TextField
{...field}
value={error ? value : (value!==null ? JSON.stringify(value) : '')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have a note there's a bug with errors and dictionary type - this line looks like it might be a good place to start debugging?

@scott-taubman
Copy link
Contributor

Layout and usability-wise I agree that we're moving in the right direction here. I'm okay merging this. If the comments made here need addressing then go ahead and do that and then merge. If we're going to address in a follow-up that's fine too. A couple of minor styling polish comments.

image

In the example above:

  • The comment box seems like it's missing a margin or padding on the top and is feels overly close to the set of fields above it. It could use some breathing room similar to the spacing between other fields.
  • The comment box and command info box are not horizontally aligned with the parameter fields, which looks odd (the parameter fields are slightly more indented).
  • The help text for the parameter fields being indented also looks a little odd. I would align the start of that text with the input field, similar to how it is in the old UI.

@scott-taubman scott-taubman merged commit da293fa into beer-garden:main May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better differentiate between optional and required parameters Additional dynamic parameter support
3 participants