-
Notifications
You must be signed in to change notification settings - Fork 163
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
Support better K8s Resource Name editing #3167
Support better K8s Resource Name editing #3167
Conversation
6364a1b
to
562adfb
Compare
frontend/src/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField.tsx
Outdated
Show resolved
Hide resolved
d375790
to
66eb345
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3167 +/- ##
==========================================
+ Coverage 85.32% 85.36% +0.03%
==========================================
Files 1270 1275 +5
Lines 27900 28010 +110
Branches 7422 7449 +27
==========================================
+ Hits 23807 23912 +105
- Misses 4093 4098 +5
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@xianli123 @kywalker-rh Please review the screenshots (they are mostly in a collapsed section, look for the expand text in the screenshot section of the description and expand it) |
Thanks @andrewballantyne . It looks great. There is one point we could discuss, and I have attached a graph below. |
@xianli123 I don't understand the desire -- why don't we limit bad characters... why even have error states? I feel the feedback is most commonly done via when they manually type into the field not to "stop them" but to inform them? (started a thread on slack -- there will be more questions about this direction) |
Reviewed this with @andrewballantyne in the slack thread. Our decision is to remove the yellow warning text when the resource name has been truncated. Once that change is made this lgtm. cc: @xianli123 |
Changes needed to page objects & UX implementation |
I agree with you on the update. @kywalker-rh. After the change, it will LGTM. |
36aed1b
to
8cd1751
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.
Dupe implementation from #3172
@xianli123 @kywalker-rh Okay, updated the screenshot in the description. New state when you type a too long display name (and we auto translate it for k8s): |
frontend/src/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField.tsx
Outdated
Show resolved
Hide resolved
Looks great, thanks @andrewballantyne! |
LGTM! Thanks @andrewballantyne . |
frontend/src/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField.tsx
Show resolved
Hide resolved
return ( | ||
<StackItem> | ||
<FormGroup {...formGroupProps} isRequired> | ||
<TextInput |
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.
Do you need id={`${dataTestId}-resourceName`}
here?
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! I had to swing back through with AXE support after I built this all from the ground up. I'll make that change 👍
safeK8sPrefix: safePrefix, | ||
}); | ||
changedData.k8sName = { | ||
value: k8sValue, |
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.
I think you might need to add a check for invalidCharacters
here.
61d7d0b
to
69ed014
Compare
Last commit addresses the special character situation. I also rebased to get rid of the conflict |
69ed014
to
1bbfe88
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.
Nice handling with most of the edge cases we can find so far.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: DaoDaoNoCode 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 |
https://issues.redhat.com/browse/RHOAIENG-583
Description
Provides a new cleaner interface to manage DisplayNames & Descriptions & K8s Name generation. This component will provide a unified front for any element we need to manage K8s items with.
ONLY REFACTORS Project modal & Workbench form. There will be more that can use this.
Screenshots
Project Modal:
Click to expand more Project Screenshots
These are in chronological order of operation.
Initial State:
Just Display Name Added:
Looking at the resource name:
Make the display name too long:
Make the resource name exceed max characters:
Throw on an invalid character:
Trim it back to the max (no auto trimmed action, so no warning):
Clear out the resource name:
Make an invalid k8s name that is short:
Make it a valid k8s name:
After submit & editing:
Workbench screen (only the workbench name is configured this way -- other resources created here are not YET):
How Has This Been Tested?
Using the Create/Edit Project Modals & the Create/Edit Workbench Forms.
Test Impact
Added unit tests for the new component. Created a Cypress Page object and refactored the Project/Workbench page objects to use it. Existing Cypress tests should cover the Project modal & Workbench form; minor adjustments made for new design.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main