-
Notifications
You must be signed in to change notification settings - Fork 605
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
Bug 1797702: remove search button and search status/result for deploy image #4187
Conversation
@sahil143: This pull request references Bugzilla bug 1797702, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
cc: @serenamarie125 @openshift/team-devconsole-ux |
@sahil143: This pull request references Bugzilla bug 1797702, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@sahil143 can we utilize the PF4 component shown here ? https://www.patternfly.org/v4/documentation/react/components/form#invalid |
32e10df
to
75e61f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I think that the icon should be in the text field itself. Although that should NOT hold up this PR from a UX perspective, because getting these usability improvements merged is high priority from my perspective!
It would appear there is a style break in the patternfly variable. The font-size-md variable is printing out I overrode the variable locally, and got it to work: Not the solution I would think... but gives a place to start investigating. Also I want to note the empty-state error is pretty bad. I am not sure if we even want to show an empty-state error? @openshift/team-devconsole-ux thoughts? |
@serenamarie125 how do you feel about the experience in the git flow? |
/kind bug |
@sahil143: This pull request references Bugzilla bug 1797702, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
75e61f4
to
8890ac1
Compare
@andrewballantyne I have updated the error for empty input field. |
Thanks @sahil143 I think that's consistent with the git flow. They probably should be in sync for their error / validating / success states. |
Fyi there's a pr by @jeff-phillips-18 that willl clash here. |
thanks for updating! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a slew of visual issues I find with the validation text (see below). We shouldn't be showing internal variable ids in error messages.
Once you get an error - as you start typing (or select a browser autocomplete value) you get this error:
I think our errors should always start with a capital letter:
If you start with an invalid character, like a /
, you see the field id:
export enum VALIDATED { | ||
default = 'default', | ||
success = 'success', | ||
error = 'error', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we go with enum variables being PascalCase and enum types being ALL_CAPS?
Not sure we actually have a standard here, but I think that aligns with other enums I am seeing in the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imported ValidatedOptions
from Patternfly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works too... note the PascalCase 😉
return ( | ||
<span style={{ fontWeight: 'bold' }}> | ||
<LoadingInline /> | ||
</span> | ||
); | ||
} | ||
if (!values.isSearchingForImage && validated === VALIDATED.success) { | ||
return ( | ||
<span style={{ fontWeight: 'bold', color: 'var(--pf-global--success-color--200)' }}> | ||
<CheckCircleIcon /> Validated | ||
</span> | ||
); | ||
} | ||
|
||
return ''; | ||
}; | ||
|
||
const getHelpTextInvalid = | ||
validated === VALIDATED.error ? ( | ||
<span style={{ color: 'var(--pf-global--danger-color--200)' }}> | ||
<ExclamationCircleIcon /> | ||
| ||
{values.searchTerm === '' ? 'Required' : values.isi.status} | ||
</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely should be putting this stuff into sass. Please create a styles file and move this content into there. Inline styles are only useful when we have dynamic values, not for static values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I highlighted this whole area because there were multiple inline styles haha
return ''; | ||
}; | ||
|
||
const getHelpTextInvalid = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables which start with get
should not be immediately evaluated... get
is typically used for a lazy-retrieval, and not for sometime that is immediately available. Suggest renaming to invalidHelpText
or something similar.
{values.searchTerm === '' ? 'Required' : values.isi.status} | ||
</span> | ||
) : ( | ||
'' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should almost never have to equate something down to an empty string. Controlled input values is the only thing I can think of... but you're creating a DOM element, which a falsy value should do fine.
Ie:
const invalidHelpText = validated === VALIDATED.error && (
<span className="odc-...">
<ExclamationCircleIcon />
{' '}
{values.searchTerm === '' ? 'Required' : values.isi.status}
</span>
);
@christianvogt do you mean PR #4268? Because this is Deploy Image, and that is Import Git. Ah, I guess you mean in the Formik elements, not the ImageSearch stuff... |
@andrewballantyne We show the same errors in current implementation as well. And In the UX doc, It specifically says |
8890ac1
to
9c4bbb3
Compare
name="searchTerm" | ||
label="Image Name" | ||
onSearch={handleSearch} | ||
placeholder="Enter an image name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sahil143 seems the removal of data-test-id="deploy-image-search-term"
willl impact integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@invincibleJai added data-test-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @invincibleJai - we need to be careful not to remove data-test-id
s.
9c4bbb3
to
2374d49
Compare
Apparently this is a server error... that sucks. Wish those could be cleaner. |
@spadgett: This pull request references Bugzilla bug 1797702, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I agree that the internal registry error handling shouldn't failed because of user input. So it's an exceptional case. Looking closer at the code, while error dialogs is the general solution to exceptional error handling, for these forms it may make more sense to keep the error closer to the input fields. For the sake of completing this PR, could we just wrap the internal registry form in a |
add validated text show field required when searchTerm is empty create scss file for image search component, use patternfly validatedOption enum add data-test-id to ImageSearch component add classes for success help text s remove styles removed funtional test because Input search button is removed and no longer used remove $ fix functional tests show inline error for ImageStream add error refactor reorder Imports
1b6406d
to
c7c4ce0
Compare
This sounds good, made the changes accordingly. If there is any we would show it inline. (Mock error in the screenshot, Not actual one) Also, did some refactor to keep it consistent after the rebase because PR #4376 made some changes to InputField. |
/bugzilla refresh |
@sahil143: This pull request references Bugzilla bug 1797702, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The design doc had two criteria, but isn't clear what the final outcome was.
|
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
@christianvogt I think we need to update Git and Deploy Image as the user types at the same time too. Perhaps we just enhance https://issues.redhat.com/browse/ODC-2451 |
/retest EDIT: Looks like a connection error on the CI server |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, jeff-phillips-18, sahil143, serenamarie125 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
/cherrypick release-4.4 |
@christianvogt: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@christianvogt: new pull request created: #4403 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@sahil143: All pull requests linked via external trackers have merged. Bugzilla bug 1797702 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ODC-bug: https://issues.redhat.com/browse/ODC-2337
This PR makes the changes suggested by UX on the Deploy image page.
default state
error state
success