Validations added for BannerLeftContent and BannerLeftAppearance of C… #561
Conversation
…reateBannerPage using formik and yup
WalkthroughThis pull request updates both frontend and backend components related to banner functionality. It integrates Formik for managing and validating form inputs in multiple banner components, revises styling in corresponding CSS/SCSS modules, enforces stricter URL validation on the backend, and introduces a new helper module for validation schemas. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BLA as BannerLeftAppearance Component
participant FM as Formik Handler
participant V as appearanceSchema (Yup)
U->>BLA: Enter color values
BLA->>FM: Set field values (backgroundColor, fontColor)
FM->>V: Validate color inputs
V -->>FM: Return validation result
alt Valid Input
FM->>BLA: Update state and render updated appearance
else Invalid Input
FM->>BLA: Display error messages
end
sequenceDiagram
participant U as User
participant BLC as BannerLeftContent Component
participant FM as Formik Handler
participant V as newBannerSchema (Yup)
participant BE as Backend Validator
U->>BLC: Enter URL and Action URL
BLC->>FM: Set field values (url, actionUrl)
FM->>V: Validate URLs
V -->>FM: Return validation result
alt Valid Input
FM->>BLC: Process form submission
BLC->>BE: (Optional) Validate URL formats on backend
BE -->>BLC: Return validation status
else Invalid Input
FM->>BLC: Display error messages
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (6)
frontend/src/scenes/banner/CreateBannerPage.jsx (1)
10-10: Naming Note: Appearance vs. Apperance!
The import statement for BannerLeftAppearance uses'./BannerPageComponents/BannerLeftAppearance/BannerLeftApperance'. Consider standardizing the spelling toBannerLeftAppearancefor consistency and clarity—no one wants to trip over a misspelt rhyme when the beat is on!frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.css (1)
16-23: Consider adding responsive width property.The fixed width of 241px might not adapt well to different screen sizes. Consider using relative units or media queries for better responsiveness.
- width: 241px; + width: 100%; + max-width: 241px;frontend/src/utils/bannerHelper.js (1)
35-42: Hex color validation should support short format (#RGB).The regex only supports 6-digit hex colors (#RRGGBB) but should also allow 3-digit format (#RGB) for convenience.
backgroundColor: Yup.string() - .matches(/^#[0-9A-Fa-f]{6}$/, 'Invalid value for Background Color') + .matches(/^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6})$/, 'Invalid value for Background Color') .required('Background Color is required'), fontColor: Yup.string() - .matches(/^#[0-9A-Fa-f]{6}$/, 'Invalid value for Font Color') + .matches(/^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6})$/, 'Invalid value for Font Color') .required('Font Color is required'),frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (1)
14-36: Consider adding enableReinitialize to Formik.When prop values change externally, Formik won't update the form values unless enableReinitialize is set to true. This is important for components receiving props from parent components.
<Formik initialValues={{ backgroundColor, fontColor }} + enableReinitialize validationSchema={appearanceSchema} validateOnBlur={false} validateOnMount={false} onSubmit={async (values, { setSubmitting }) => {frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (2)
43-49: Consider adding form submission handlingYour Formik setup looks good with initialValues and schema validation, but I don't see an onSubmit handler. Is this form meant to be submitted somewhere? If not, that's cool, but if it is - you'll need to add that handler.
Also, you've got validateOnBlur set to false but then manually trigger validation in onBlur handlers - might wanna just set validateOnBlur to true instead.
<Formik initialValues={{ url, actionUrl }} validationSchema={newBannerSchema} enableReinitialize={true} validateOnMount={false} - validateOnBlur={false} + validateOnBlur={true} >
99-113: Consider unifying state management with FormikYour implementation works, but you're maintaining two sources of truth - Formik's internal state and your component props state. This could lead to synchronization issues.
You might want to let Formik handle the form state completely and update your parent component only when necessary (like on submit or field blur).
<CustomTextField TextFieldWidth="241px" error={!!errors.actionUrl} value={actionUrl} name="actionUrl" - onChange={handleSetActionUrl} + onChange={(e) => { + handleSetActionUrl(e); + handleChange(e); // Add Formik's handleChange + }} onBlur={(e) => { handleBlur(e); - validateField('actionUrl'); }} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (10)
.env(1 hunks)docker-compose.yml(2 hunks)frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.css(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.css(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.module.scss(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx(3 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.module.scss(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(5 hunks)frontend/src/utils/bannerHelper.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (19)
.env (1)
2-2: ENV Configuration Updated to Development!
SwitchingNODE_ENVfrom production to development sets the proper vibe for debugging—just like freestyling under pressure when your palms are sweaty. Everything looks solid here.docker-compose.yml (2)
55-57: Frontend Volumes Commented Out!
Commenting out the volume mappings for the frontend service is a deliberate move to avoid syncing local directories inside the container. Just ensure this aligns with your development workflow—keep it smooth like a tight rap verse even if your knees are weak!
70-75: Updated Frontend Startup Command!
The conditional logic in thebash -ccommand now accurately distinguishes between the development and non-development environments. Just make sure the beat drops exactly as you expect when switching contexts.frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.css (1)
30-30: End-of-File Newline Added!
Though a small change, ending the file with a newline keeps things clean and prevents potential diff noise—like keeping your flow crisp when every line counts.frontend/src/scenes/banner/CreateBannerPage.jsx (4)
27-30: Default State Values Set with a Strong Beat!
InitializingurlandactionUrlwith'https://'(and setting default strings for actions) ensures your form’s initial values are in tune with the new validation schema. Keep that flow tight, just like a smooth verse.
47-54: Fetched Banner Data Mode: On Point!
The useEffect handling for edit mode now sets state values with safe fallbacks. When the pressure is high, these defaults are your insurance—ensure they harmonize with your Yup schema inbannerHelper.js.
70-82: onSave Logic Drops the Beat!
TheonSavefunction compiles bannerData with clear, conditional values. Lower-casing button actions and setting toast messages makes the output as crisp as a well-delivered rap—just double-check that the messages match your intended tone.
90-129: Component Rendering is Ready to Rock!
Your JSX layout inBannerPageutilizes GuideTemplate smartly, separating preview and form content with fluid transitions. Everything meshes like a perfectly timed rhyme—ensure data flows as expected when the spotlight hits.frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.module.scss (1)
3-18: Container Rework Brings the Style!
The newly structured.containerclass and its nested elements now flex and align like a crew in perfect sync. This rework supports the updated Formik form handling, keeping your style as fresh as a new beat drop.frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.css (2)
1-7: Good organization of container styling.The flex container setup for banner appearance is well structured with appropriate direction and alignment properties. Using CSS variables like
--main-text-coloris a good practice for maintaining consistency across the application.
25-33: Well-structured error styling.The error message styling is well thought out with appropriate font properties and color. The absolute positioning ensures the error messages don't disrupt the layout flow.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.module.scss (1)
3-20: Nice SCSS organization with proper nesting.The restructured container class with nested selectors follows good SCSS practices. This improves code readability and maintenance.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (2)
44-47: Good synchronization between Formik and parent state.The component properly synchronizes Formik's internal state with the parent component's state by calling both setFieldValue and the respective setter functions from props.
Also applies to: 63-66
52-56: Error handling implementation is well done.The conditional rendering of error messages with appropriate styling provides good user feedback when validation fails.
Also applies to: 71-75
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (5)
7-8: Good addition of form validation librariesNice job adding Formik and the validation schema! These are great tools for form management that'll make validation way more robust.
50-81: Form structure looks clean and well-organizedThe Form component structure is looking solid. You've organized the repetition, action, and position controls in a logical way.
84-97: URL field validation implemented properlyGood job implementing validation for the URL field with proper error handling. Just make sure the "error-message" class is defined somewhere in your CSS.
114-117: Form structure wrapped up nicelyThe closing tags are properly placed, and the overall structure of your Formik implementation looks clean.
132-132: PropType definition is correctYou've properly defined the buttonRepetition prop type as a string, which matches how it's used in the component.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx
Show resolved
Hide resolved
|
UI wise looks good to me. What is all about the skeleton here? For some reason I missed that part previously. 
|
|
This skeleton reflects as if there is content on the page and the banner will either be on top or bottom w.r.t the content. |
There was a problem hiding this comment.
Make sure that you fork from develop and open your PRs to the develop branch.
Because of a validation in banner backend for Action Url we get 'Relative URL must start with /' error when we start with https://. Can you also change that validation to accept urls that start with https:// and http:// ?
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.css
Outdated
Show resolved
Hide resolved
@erenfn So, I tried implementing the validation to accept url's starting with https:// and http:// which is absolute and relative respectively, what I found is irrespective of the validation logic in frontend, we are only able to pass either absolute or relative url in the two fields. So, I inspected the logic implemented for backend and what I found is
Solution to this:
Issues Addressed:
Waiting for your approval to fix it in any way you want and push the final code. |
|
Then let's go with:
|
…file fixed, BannerLeftAppearance.module.css file added
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/utils/bannerHelper.js (2)
42-53: 🛠️ Refactor suggestionForm validation ain't conditional based on action type - this ain't right
Your schema's validatein' URLs regardless of the action selected. If someone picks 'no action', they shouldn't need to provide valid URLs, ya feel me? Knees weak, arms heavy with unnecessary validation.
const newBannerSchema = Yup.object().shape({ action: Yup.string().oneOf( ['no action', 'open url', 'open url in a new tab'], 'Invalid value for action' ), url: Yup.string() + .when('action', { + is: (action) => action !== 'no action', + then: () => Yup.string() .test('url', 'Invalid value for URL', validateUrl) .max(2000, 'URL must be at most 2000 characters'), + otherwise: () => Yup.string() + }), actionUrl: Yup.string() + .when('action', { + is: (action) => action === 'open url' || action === 'open url in a new tab', + then: () => Yup.string() .test('actionUrl', 'Invalid value for Action URL', validateUrl) .max(2000, 'URL must be at most 2000 characters'), + otherwise: () => Yup.string() + }), });
32-40:⚠️ Potential issueYo, the URL validation logic needs a check for empty strings, dawg
The
validateUrlfunction doesn't handle empty strings which could make your form validation choke. If a user submits without entering a URL, this function's gonna throw an error faster than dropping mom's spaghetti.const validateUrl = (url) => { + if (!url) return false; try { new URL(url); return true; } catch { return RELATIVE_URL_REGEX.test(url); } };frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (1)
19-27:⚠️ Potential issueThe
onSavefunction is missing like mom's spaghetti - it'll crash at submit timeYou're calling
onSave()in the form submission handler, but this function ain't defined anywhere! Your app's gonna throw up an error when someone tries to submit this form.onSubmit={async (values, { setSubmitting }) => { try { - onSave(); + // Either define onSave function or handle submission directly + // If onSave should be a prop: + if (typeof onSave === 'function') { + onSave(values); + } } catch (error) { console.error(error); } finally { setSubmitting(false); } }}
🧹 Nitpick comments (3)
frontend/src/utils/bannerHelper.js (1)
3-30: Clean up your commented code, bro - it's like old spaghetti stainsThere's a ton of commented-out code versions of URL validation. It's making this file harder to read than rap lyrics without punctuation. Keep your code clean by removing these unused validation attempts.
Consider removing all these commented-out functions and regexes since you've settled on an implementation.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (1)
7-12: Props documentation would be nice - what's the expected format?The component takes several props but there's no documentation about their expected types or formats. Adding some JSDoc comments would help other devs understand what to pass in without their knees getting weak trying to figure it out.
/** + * @param {string} backgroundColor - Hex color code for background + * @param {Function} setBackgroundColor - Function to update background color + * @param {string} fontColor - Hex color code for font + * @param {Function} setFontColor - Function to update font color + */ const BannerLeftAppearance = ({ backgroundColor, setBackgroundColor, fontColor, setFontColor, }) => {backend/src/utils/banner.helper.js (1)
19-28: URL validation could be DRYerYou've got similar validation logic for both URL and actionUrl. Could refactor this to use the existing
validateRelativeUrlfunction that's already defined later in the file (lines 73-83).Consider reusing the existing
validateRelativeUrlfunction to avoid duplicated validation logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (6)
backend/src/utils/banner.helper.js(1 hunks)docker-compose.yml(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.module.css(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(5 hunks)frontend/src/utils/bannerHelper.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftAppearance.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- docker-compose.yml
- frontend/src/scenes/banner/CreateBannerPage.jsx
🔇 Additional comments (5)
frontend/src/utils/bannerHelper.js (1)
55-63: You got commented-out optional validation too - decide what you wantThere's a commented-out version of the appearance schema that makes fields optional. Based on PR comments about flexibility, maybe you actually want this version? Your palms are sweaty trying to decide between required and optional fields.
Double-check with your team whether the appearance fields should be required or optional based on the discussion in PR comments.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (2)
5-5: Check your CSS file extension, homie - module.css vs module.scssYou're importing from 'BannerLeftAppearance.module.css' but the PR summary mentions 'BannerLeftAppearance.css'. Make sure your import path matches the actual file name or your styles are gonna be missing like lyrics you forgot.
Verify that the file 'BannerLeftAppearance.module.css' exists in the correct location.
14-79: Form doesn't actually submit anything - what's the point?The Formik setup has an onSubmit handler but there's no submit button and the form doesn't seem to be submitted anywhere. Is this intentional? Form's heavy with validation but has no way for users to actually submit it.
Is this form meant to auto-save? If so, consider using Formik's
enableReinitializeprop and watching for value changes instead of having an unused submit handler.backend/src/utils/banner.helper.js (2)
31-42: Action URL validation looks good for both relative and absolute URLsThe validation for actionUrl properly handles both relative and absolute URLs, allowing for flexibility as discussed in the PR comments. You're not throwing up on your sweater with this implementation.
32-42: Validation inconsistency between URL and actionUrlURL is required but actionUrl is optional. Based on the PR discussion, this seems intentional, but the inconsistency in how they're validated might cause confusion. Your palms are gonna get sweaty debugging this later.
Verify this behavior is intentional based on this PR comment: "URL should be required while the Action URL could remain optional, with both fields allowing for relative and absolute URLs."
|
@erenfn
|
|
Can be merged after resolving the conflicts. Most of them will be just styling differences |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (1)
15-28:⚠️ Potential issueUndefined onSave function will cause runtime error.
There's a call to
onSave()in the form submission handler, but this function isn't defined anywhere in the component. This will throw an error when the form is submitted.onSubmit={async (values, { setSubmitting }) => { try { - onSave(); + // Either define onSave function or handle submission directly + // If onSave should be a prop: + if (typeof onSave === 'function') { + onSave(values); + } } catch (error) { console.error(error); } finally { setSubmitting(false); } }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (2)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/scenes/banner/CreateBannerPage.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (5)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx (5)
1-6: Clean imports, but let's check the appearance schema.The imports look well-organized, importing the required components and utilities. The appearanceSchema is now properly imported from the bannerHelper utility.
8-13: Props structure looks good.The component accepts the necessary props for managing banner appearance - backgroundColor, setBackgroundColor, fontColor, and setFontColor. These props are clearly defined and follow React's pattern for controlled components.
30-37: Good use of Formik's render props pattern.The component correctly extracts the necessary Formik properties to handle form state, errors, and interactions.
38-58: Background color field implementation looks good.The ColorTextField for background color is properly implemented with Formik integration. The onChange handler correctly updates both the Formik state and the parent component state. Error handling is also properly implemented.
59-77: Font color field implementation looks good.Similar to the background color field, the font color field is also properly implemented with Formik integration and error handling.
frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx
Show resolved
Hide resolved
|
…reateBannerPage using formik and yup
Describe your changes
Added form and schema validation to BannerLeftContent.jsx and BannerLeftAppearance.jsx components using formik and yup.
New components created:
Components modified:
Issue number
#455
Screenshots attached: