456 formik and yup validations for popup fe #529
Conversation
WalkthroughThis pull request introduces comprehensive changes to the popup management system, focusing on state management, form validation, and migration updates. The modifications span multiple frontend components and a backend migration script, implementing Formik and Yup for enhanced form handling and validation. The changes consolidate state management, introduce prop type checking, and update the database migration to support new popup configuration options. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
backend/migrations/0008-1.0-popup.js (2)
Line range hint
89-108: Mom's spaghetti moment: Missing data validation in bulk update!The bulk update operation doesn't validate the ENUM values before updating. This could lead to invalid data if the source values don't match the new ENUM constraints.
Add validation before the bulk update:
if (allPopups.length > 0) { + const validCloseActions = ['no action', 'open url', 'open url in a new tab']; + const validSizes = ['small', 'medium', 'large']; + const validRepetitionTypes = ['show only once', 'show every visit']; + const updates = allPopups.map((val) => ({ id: val.id, - closeButtonAction: val.closeButtonAction, - popupSize: val.popupSize, - repetitionType: val.repetitionType, + closeButtonAction: validCloseActions.includes(val.closeButtonAction.toLowerCase()) + ? val.closeButtonAction.toLowerCase() + : 'no action', + popupSize: validSizes.includes(val.popupSize.toLowerCase()) + ? val.popupSize.toLowerCase() + : 'small', + repetitionType: validRepetitionTypes.includes(val.repetitionType.toLowerCase()) + ? val.repetitionType.toLowerCase() + : 'show only once', }));🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
Line range hint
125-137: Weak knees alert: Down migration needs cleanup!The down migration comment refers to 'guide_logs' table instead of 'popups'. Also, it should clean up the ENUM types.
Update the down migration:
down: async (queryInterface, Sequelize) => { const transaction = await queryInterface.sequelize.transaction(); try { - // Drop the guide_logs table + // Drop the popups table and clean up ENUM types await queryInterface.dropTable(TABLE_NAME, { transaction }); + await queryInterface.sequelize.query( + 'DROP TYPE IF EXISTS enum_popups_closeButtonAction;' + + 'DROP TYPE IF EXISTS enum_popups_popupSize;' + + 'DROP TYPE IF EXISTS enum_popups_repetitionType;', + { transaction } + );🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🧹 Nitpick comments (7)
frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.jsx (3)
9-14: Great shift to the new prop signature!
Swapping outpopupSizeforpopupAppearanceandonSaveclarifies your intent. Make sure to double-check you’ve cleaned up any references to the old props, so you don’t trip over leftover spaghetti.
20-37: Formik integration looks solid, but consider user-facing errors.
Your submission flow setssetSubmitting(false), which is good, but also think about gently letting the user know if something goes sideways—kind of like a quick heads-up if your knees get wobbly.
97-107: PropTypes are well-defined, but consider shaping them further.
popupAppearance: PropTypes.object.isRequiredcould become a more explicit shape if you want that extra bit of code confidence, helping ensure no sneaky sauce sneaks past your validation.frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (1)
20-38: Formik’s onSubmit logic is clear, but consider capturing errors.
You’re usingonSave()inside a try-catch, which is sweet. Don’t forget a user-facing cue if the saving fails—keeps the spaghetti off your sweater.frontend/src/utils/popupHelper.js (1)
4-4: Knees weak, arms heavy: Let's support more color formats! 🎨The current color validation only supports 6-digit hex colors. Consider supporting:
- 3-digit hex colors (#RGB)
- RGB/RGBA format
- HSL/HSLA format
-const COLOR_REGEX = /^#[0-9A-Fa-f]{6}$/; +const COLOR_REGEX = /^(?:#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}|rgb\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*\)|rgba\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*,\s*(?:0|1|0?\.\d+)\s*\)|hsl\(\s*\d+\s*,\s*\d+%\s*,\s*\d+%\s*\)|hsla\(\s*\d+\s*,\s*\d+%\s*,\s*\d+%\s*,\s*(?:0|1|0?\.\d+)\s*\))$/;Also applies to: 38-61
frontend/src/products/Popup/PopupComponent.jsx (1)
67-67: These styles making me nervous! Move 'em to CSS! 🎨Extract hardcoded styles to the CSS module for better maintainability:
-style={{ marginLeft: '5px' }} +className={styles.headerText} -style={{ color: '#98A2B3', fontSize: '20px', cursor: 'pointer' }} +className={styles.closeIcon} -style={{ - borderRadius: '8px', -}} +className={styles.actionButton}Add to your CSS module:
.headerText { margin-left: 5px; } .closeIcon { color: var(--gray-400); font-size: 20px; cursor: pointer; } .actionButton { border-radius: 8px; }Also applies to: 69-69, 84-84
frontend/src/scenes/popup/CreatePopupPage.jsx (1)
110-126: Vomit on his sweater already: Error handling needs improvement!The
onSavefunction should handle specific error cases and provide more informative error messages.Enhance error handling:
const onSave = async () => { + try { + // Validate URLs before saving + await urlValidationSchema.validate({ + url: popupContent.url, + actionButtonUrl: popupContent.actionButtonUrl, + }); + const popupData = { repetitionType: popupContent.buttonRepetition.toLowerCase(), closeButtonAction: popupContent.buttonAction.toLowerCase(), url: popupContent.url, actionUrl: popupContent.actionButtonUrl, actionButtonText: popupContent.actionButtonText, headerBackgroundColor: popupAppearance.headerBackgroundColor, headerColor: popupAppearance.headerColor, textColor: popupAppearance.textColor, buttonBackgroundColor: popupAppearance.buttonBackgroundColor, buttonTextColor: popupAppearance.buttonTextColor, popupSize: popupAppearance.popupSize.toLowerCase(), header, content, }; + } catch (validationError) { + emitToastError(`Validation failed: ${validationError.message}`); + return; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/migrations/0008-1.0-popup.js(1 hunks)frontend/src/components/ColorTextField/ColorTextField.jsx(1 hunks)frontend/src/products/Popup/PopupComponent.jsx(5 hunks)frontend/src/scenes/popup/CreatePopupPage.jsx(7 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.jsx(1 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.module.scss(1 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx(1 hunks)frontend/src/utils/popupHelper.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (10)
frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.jsx (3)
2-3: Fantastic usage of PropTypes and Formik imports!
This helps reduce runtime slip-ups and keeps your code extra sturdy, going strong like you’ve finally got that sweater under control, no spaghetti spillage here.
15-19: Initial values generation is smooth!
Building the form’s initial structure from thedataarray is a sweet approach that keeps everything dynamic with minimal fuss.
38-92: Efficient field rendering with dynamic updates.
Mapping overdataand leveragingFormikplusonBluris top-notch. Just keep in mind that frequent state updates can sometimes cause performance jitters. This approach should be fine, but keep an eye out if you start to see the sauce splattering.frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (3)
1-3: Smooth import statements, continuing the same approach.
Using Formik with PropTypes keeps everything consistent, so that your popup content form packs a real punch without mess.
40-146: Comprehensive form fields with setPopupContent integration.
This is consistent withPopupAppearanceusage of Formik. The synergy is excellent, preventing too many lumps in the sauce. Keep the user experience smooth by showing helpful messages when validations fail.
157-158: Cleaned-up prop definitions deserve a nod.
You’ve tidied up old props and introducedsetPopupContent, which helps ensure you’re not juggling too many repeating utensils. Bravo for reducing clutter!frontend/src/components/ColorTextField/ColorTextField.jsx (1)
6-8: New props ensure Formik compatibility.
AddingnameandonBlurmeans this component can follow your form’s steps gracefully, leaving your hands free of saucy mistakes. It’s a handy improvement that fosters consistent usage across your forms.frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.module.scss (1)
22-22: Yo, this container class closure is on point! 🔥Clean and properly indented closure of the container class.
backend/migrations/0008-1.0-popup.js (1)
3-3: Yo dawg, table name change needs careful handling!The table name change from 'popup' to 'popups' could break existing code that references the old table name. Make sure all queries and models are updated accordingly.
Let's check for any remaining references to the old table name:
frontend/src/scenes/popup/CreatePopupPage.jsx (1)
162-167: LGTM: Clean styling implementation!The styling for RichTextEditor is well-structured and uses consistent spacing.
frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.module.scss
Outdated
Show resolved
Hide resolved
It looks good on my end though!
Here is the code that renders it-
|
|
I think it is somehow position absolute. Before scrolling down: |
Describe your changes
Issue number
#456
Please ensure all items are checked off before requesting a review: