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

Display errors in SUSE Manager settings form #2455

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

dottorblaster
Copy link
Contributor

@dottorblaster dottorblaster commented Mar 21, 2024

Description

Displaying validation errors in the SUSE Manager settings modal.

How was this tested?

Jest tests, Storybook

@dottorblaster dottorblaster added the enhancement New feature or request label Mar 21, 2024
@dottorblaster dottorblaster self-assigned this Mar 21, 2024
@dottorblaster dottorblaster changed the title Display errors in suse manager settings form Display errors in SUSE Manager settings form Mar 21, 2024
@dottorblaster dottorblaster marked this pull request as ready for review March 22, 2024 12:07
onClearErrors();
}}
/>
<div className="col-span-4">
Copy link
Member

Choose a reason for hiding this comment

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

What about embedding the error in the input component?

<Input
  value={url}
  placeholder="Enter a URL"
  error={getError('url', errors)}
  onChange={({ target: { value } }) => {
    setUrl(value);
    onClearErrors();
  }}
/>

Really up to you 😄 There are just many hasError and related getError.
I am fine either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something I'd be up to but it would tie us to one single implementation of error visualization and I want to wait instead for more form implementations to see what we settle upon.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

There is a tiny issue when going back and forth, see the gif.

suma-validation-error

In general looks good. If we need nicer validation messages we'll change them in ecto.

@dottorblaster dottorblaster force-pushed the display-errors-suse-manager-settings branch from b1abc6e to e816e64 Compare March 25, 2024 09:37
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

LGTM

@dottorblaster dottorblaster merged commit 6f651ec into main Mar 25, 2024
24 checks passed
@dottorblaster dottorblaster deleted the display-errors-suse-manager-settings branch March 25, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants