Bug 1777256: Removed disable button logic to achieve overall consistency #3500
Conversation
bipuladh
commented
Nov 20, 2019
•
bipuladh
commented
- Removed submit button disabling logic.
openshift-ci-robot
added
size/M
| @@ -88,6 +89,19 @@ const awsRegions = [ | |||
|
|
|||
| const awsRegionItems = _.zipObject(awsRegions, awsRegions); | |||
|
|
|||
| const nameLabelTooltip = ( | |||
There was a problem hiding this comment.
We have a ton of forms that accept a name, but don't have a tooltip like this anywhere else. I think we should apply this generally everywhere or leave it out. The problem is that different resources can have different rules.
There was a problem hiding this comment.
@shirimordechay WDYT? Would it be okay for us to leave the tooltip out?
There was a problem hiding this comment.
The question is if those other places also have a rule list that might cause error messages...
What is worst? to add those tooltip details ahead and let the user know what is a valid name (and prevent errors) or add a new type of tooltip? If the user won't have those details anywhere he can make one mistake after another and continuously receive error messages. This can be very frustrating. Showing the requirements/ rules ahead is a great way to prevent errors and frustrations.
Is there any other implementation that handles this kind of situation?
There was a problem hiding this comment.
IMO the error messages will guide the user to come up with the correct value for a field.
There was a problem hiding this comment.
Yes, eventually the user should make it but what if the user types a name that has 3-4 different errors? will we show all those errors at once or only the first invalid rule?
In any way, best is to prevent errors since the console error messages are not very informative at the moment and may not guide the user so well.
There was a problem hiding this comment.
@spadgett your thoughts? I believe we should let the user try without any aid( assuming that an administrator using these features s/he would be able to decode the error).
There was a problem hiding this comment.
@shirimordechay whatever we decide, we shouldn't do it for just one form since this is an issue for every form. that's my point. we should be consistent.
if we make a change, it should be a separate PR that updates all forms that have a name input
it would be even better if we can use the open api description so that the help text is always accurate. we've gotten out of sync in the past and shown a client hint that was not longer current with the api. that is arguably worse.
i believe we have some design stories to look at how to show field-level help in forms. we are already showing the open api doc in details pages. we should try to be consistent in how we show it for forms as well. see #2926
i'd rather solve this generally for all forms as a separate story
There was a problem hiding this comment.
Yes, I agree there should be a general solution for this type of issue. I would try to raise this issue at the UXD level so that hopefully this could be a reuse component in the future.
@spadgett Where do we have other multi requirements naming in the console?
@bipuladh Can we at least make sure that the error messages are clear enough? or this is related to the other issue we talked about?
There was a problem hiding this comment.
@shirimordechay Yeah you're right it's related to the other issue(it's send by the operator).
bipuladh
force-pushed
the
info-icon-fix
branch
from
5285eb9
to
0c3f2a0
Compare
openshift-ci-robot
added
size/S
|
@spadgett removed the field-level help. Only deals with removing disable feature for the form. |
|
/retest |
1 similar comment
|
/retest |
|
can you add the bugzilla to this. |
|
/kind bug |
openshift-ci-robot
added
the
kind/bug
bipuladh
changed the title
|
@bipuladh: This pull request references Bugzilla bug 1777256, 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. |
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
@bipuladh: This pull request references Bugzilla bug 1777256, 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. |
|
/bugzilla refresh |
|
@bipuladh: This pull request references Bugzilla bug 1777256, 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. |
openshift-ci-robot
added
bugzilla/valid-bug
bipuladh
force-pushed
the
info-icon-fix
branch
from
0c3f2a0
to
078c493
Compare
openshift-ci-robot
added
size/M
bipuladh
force-pushed
the
info-icon-fix
branch
from
078c493
to
098efc8
Compare
openshift-ci-robot
removed
the
size/M
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-gcp-console |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@bipuladh: All pull requests linked via external trackers have merged. Bugzilla bug 1777256 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. |
|
/cherrypick release-4.3 |
|
@bipuladh: new pull request created: #4935 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. |
