-
Notifications
You must be signed in to change notification settings - Fork 361
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
upcoming: [M3-8364] - Use React Hook Form instead of Formik for Create Bucket #10699
Conversation
Coverage Report: ✅ |
@@ -11,7 +11,7 @@ interface Props { | |||
onBlur: (e: any) => void; | |||
onChange: (value: string) => void; | |||
required?: boolean; | |||
selectedCluster: string; | |||
selectedCluster: string | undefined; |
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.
The RegionSelect for which this is a wrapper has the value
as string | undefined
which I believe to be correct.
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.
Create Bucket functionality is working as expected for Gen1 and legacy.
- Able to create a new bucket for Gen1 and legacy.
- Validation is working as expected for label that's already used.
- Confirming on error with helper text under label input field
- - Confirming form prevents submission if there are any validation errors.
Note: Since we are using V2 flag, we should enable the flag in LD for Gen1 in respective environment when this is shipped to production.
packages/manager/src/features/ObjectStorage/BucketLanding/CreateBucketDrawer.tsx
Outdated
Show resolved
Hide resolved
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.
On an account without Object Storage enabled, I wasn't able to enable Object Storage
Screen.Recording.2024-07-25.at.9.10.11.AM.mov
setError, | ||
watch, | ||
} = useForm<CreateObjectStorageBucketPayload>({ | ||
context: { buckets: bucketsData?.buckets ?? [] }, |
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.
Neat. Did not know about context
I need to fix the OBJ test since I'm now disabling @hana-linode The clone-linode.spec.ts is looking for the old region format, surprised this wasn't caught in the PR where we changed this? 🤔 clone-linode.spec.ts.mp4 |
I still need to test this with the multi-cluster feature flag turned on. The legacy experience worked as expected and did not see any visual regressions. Nothing in the code stood out on first inspection. |
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.
Looks good!
@jaalah tests were not updated since we're still going to display the old format for now since Gecko LA/GA got pushed 2 months (new format is feature flagged under Gecko GA). The tests passed on my PR 🤔 |
@jaalah-akamai @hana-linode
|
@carrillo-erik Yea this issue is stemming from #10702. We are looking into a patch to fix this without having to revert the work. Until then if you need to get your tests passing, you can disable the
|
packages/manager/cypress/e2e/core/objectStorage/enable-object-storage.spec.ts
Outdated
Show resolved
Hide resolved
.test( | ||
'unique-label', | ||
'A bucket with this label already exists in your selected region', | ||
(value, context) => { | ||
const { cluster, region } = context.parent; | ||
const buckets = context.options.context?.buckets; | ||
|
||
if (!Array.isArray(buckets)) { | ||
// If buckets is not an array, assume the label is unique | ||
return true; | ||
} | ||
|
||
return !buckets.some( | ||
(bucket) => | ||
bucket.label === value && | ||
(bucket.cluster === cluster || bucket.region === region) | ||
); | ||
} | ||
), |
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.
In my opinion, I think we should just remove this logic and rely on API errors for uniqueness, but this seems fine!
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.
Hmm, I haven't looked too deeply into this yet, but it seems that no API error is caught in the case of a duplicate. Instead, it silently reverts the optimistic UI rendering of the bucket or something similar. I'll investigate this further and try to address it in my next PR. For now, I think the validation is useful. 👍
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.
Ohh, got it! Thanks for the investigation 🙏
!watchLabel || | ||
!watchRegion || |
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.
From previous cafe discussion, UX decided that we should to keep form submit buttons enabled globally so that the user can submit at any time and receive validation errors.
Not a huge deal here because this is a small form, but this will diverge from our pattern.
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.
Ok no problem, I might have forgotten we talked about this 👍
Tested and verified all workflows related to these changes. In addition, all tests are now ✅. |
Description 📝
Switching from Formik to React Hook Form before updating the drawer with the new UI / API requirements.
Changes 🔄
isFeatureEnabledV2
instead of deprecatedisFeatureEnabled
onExited
Target release date 🗓️
N/A
Preview 📷
N/A
How to test 🧪
Note
Ensure you test both legacy and multi-cluster (gen1) by toggling flag
Verification steps
As an Author I have considered 🤔
Check all that apply