-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add new formik input for file size, update page media module #791
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
base: master
Are you sure you want to change the base?
Conversation
…media Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughA new MuiFormikFilesizeField component is introduced to handle file size input in megabytes while internally storing values as bytes. The component integrates with Formik and Material-UI, includes comprehensive tests covering conversion and edge cases, and is implemented in the media request module to replace the previous text-based field. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/mui/formik-inputs/mui-formik-file-size-field.js`:
- Line 19: initialValues[name] is incorrect for nested/formik paths like
"modules[0].max_file_size"; use Formik's getIn to read nested values. Replace
the emptyValue computation to use getIn(initialValues, name) (from
useFormikContext) so emptyValue becomes null when the nested initial value is
null and "" otherwise; update any references in mui-formik-file-size-field.js
(the emptyValue variable and its usage) accordingly.
- Around line 14-17: displayValue computation does not guard against undefined
leading to NaN; update the conditional used to compute displayValue in
mui-formik-file-size-field.js (the expression using field.value, BYTES_PER_MB
and Math.floor) to explicitly check for undefined (e.g., ensure field.value is
not null, not undefined, and not an empty string) before performing
Math.floor(field.value / BYTES_PER_MB) so that when field.value is undefined you
return "" instead of computing and producing NaN.
🧹 Nitpick comments (2)
src/components/mui/formik-inputs/mui-formik-file-size-field.js (2)
48-52: Prefere.preventDefault()overe.nativeEvent.preventDefault().Using
e.nativeEventis unconventional. React's synthetic event methods (e.preventDefault(),e.stopPropagation()) are sufficient and more idiomatic.Proposed fix
onKeyDown={(e) => { if (BLOCKED_KEYS.includes(e.key)) { - e.nativeEvent.preventDefault(); - e.nativeEvent.stopImmediatePropagation(); + e.preventDefault(); + e.stopPropagation(); } }}
33-61: Consider addingrequiredprop support for consistency.Looking at
MuiFormikTextFieldin the relevant snippets, it supports arequiredprop that appends*to the label. This component doesn't forward that behavior explicitly, though it may work via...propsspread.
| const displayValue = | ||
| field.value !== null && field.value !== "" | ||
| ? Math.floor(field.value / BYTES_PER_MB) | ||
| : ""; |
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.
Missing undefined check causes NaN display.
The condition field.value !== null && field.value !== "" evaluates to true when field.value is undefined, causing Math.floor(undefined / BYTES_PER_MB) to return NaN.
Proposed fix
const displayValue =
- field.value !== null && field.value !== ""
+ field.value !== null && field.value !== undefined && field.value !== ""
? Math.floor(field.value / BYTES_PER_MB)
: "";🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-file-size-field.js` around lines
14 - 17, displayValue computation does not guard against undefined leading to
NaN; update the conditional used to compute displayValue in
mui-formik-file-size-field.js (the expression using field.value, BYTES_PER_MB
and Math.floor) to explicitly check for undefined (e.g., ensure field.value is
not null, not undefined, and not an empty string) before performing
Math.floor(field.value / BYTES_PER_MB) so that when field.value is undefined you
return "" instead of computing and producing NaN.
| ? Math.floor(field.value / BYTES_PER_MB) | ||
| : ""; | ||
|
|
||
| const emptyValue = initialValues[name] === null ? null : ""; |
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.
Direct property access fails for nested field names.
initialValues[name] doesn't work when name is a path like modules[0].max_file_size. In the actual usage (see page-template-media-request-module.js), buildFieldName produces nested paths, so this will always return undefined, defaulting emptyValue to "" regardless of the actual initial value.
Use Formik's getIn helper which is already imported via useFormikContext.
Proposed fix
-import { useField, useFormikContext } from "formik";
+import { useField, useFormikContext, getIn } from "formik";- const emptyValue = initialValues[name] === null ? null : "";
+ const emptyValue = getIn(initialValues, name) === null ? null : "";🤖 Prompt for AI Agents
In `@src/components/mui/formik-inputs/mui-formik-file-size-field.js` at line 19,
initialValues[name] is incorrect for nested/formik paths like
"modules[0].max_file_size"; use Formik's getIn to read nested values. Replace
the emptyValue computation to use getIn(initialValues, name) (from
useFormikContext) so emptyValue becomes null when the nested initial value is
null and "" otherwise; update any references in mui-formik-file-size-field.js
(the emptyValue variable and its usage) accordingly.
ref: https://app.clickup.com/t/86b8gnk8b
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Tests