fix: add form validation for survey's colors properties #28450
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added form validation for CSS color properties in survey forms to prevent rendering issues, with visual error feedback for invalid color inputs.
- Added
validateColorfunction insurveyLogic.tsxusingCSS.supports()to validate color values before rendering - Created reusable
LemonFieldErrorcomponent inLemonField.tsxfor consistent error display across forms - Added
.ignore-error-borderclass inLemonInput.scssto allow selective opt-out of error border styling - Implemented validation for all color inputs (background, border, buttons) in
SurveyCustomization.tsxwith error messages
4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: +5 B (0%) Total Size: 1.18 MB ℹ️ View Unchanged
|
|
@lucasheriques does this validation also work for the settings page? |
| {tab === SurveysTabs.Settings && ( | ||
| <> | ||
| <div className="flex items-center gap-2 mb-2"> | ||
| <LemonField.Pure className="mt-2" label="Appearance"> | ||
| <span>These settings apply to new surveys in this organization.</span> | ||
| </LemonField.Pure> | ||
|
|
||
| <div className="flex-1" /> | ||
| {globalSurveyAppearanceConfigAvailable && ( | ||
| <LemonButton | ||
| type="primary" | ||
| onClick={() => { | ||
| updateCurrentTeam({ | ||
| survey_config: { | ||
| ...currentTeam?.survey_config, | ||
| appearance: { | ||
| ...currentTeam?.survey_config?.appearance, | ||
| ...editableSurveyConfig, | ||
| }, | ||
| }, | ||
| }) | ||
| }} | ||
| > | ||
| Save settings | ||
| </LemonButton> | ||
| )} | ||
| </div> | ||
| <LemonDivider /> | ||
| <div className="flex gap-2 mb-2 align-top"> | ||
| <Customization | ||
| key="survey-settings-customization" | ||
| appearance={editableSurveyConfig} | ||
| hasBranchingLogic={false} | ||
| customizeRatingButtons={true} | ||
| customizePlaceholderText={true} | ||
| onAppearanceChange={(appearance) => { | ||
| setEditableSurveyConfig({ | ||
| ...editableSurveyConfig, | ||
| ...appearance, | ||
| }) | ||
| setTemplatedSurvey({ | ||
| ...templatedSurvey, | ||
| ...{ appearance: appearance }, | ||
| }) | ||
| }} | ||
| /> | ||
| <div className="flex-1" /> | ||
| <div className="mt-10 mr-5 survey-view"> | ||
| {globalSurveyAppearanceConfigAvailable && ( | ||
| <SurveyAppearancePreview survey={templatedSurvey} previewPageIndex={0} /> | ||
| )} |
There was a problem hiding this comment.
no functionality changes here, just moved this logic to its own component
| const updateSurveySettings = (): void => { | ||
| const errors = { | ||
| backgroundColor: validateColor(editableSurveyConfig.backgroundColor, 'background color'), | ||
| borderColor: validateColor(editableSurveyConfig.borderColor, 'border color'), | ||
| ratingButtonActiveColor: validateColor( | ||
| editableSurveyConfig.ratingButtonActiveColor, | ||
| 'rating button active color' | ||
| ), | ||
| ratingButtonColor: validateColor(editableSurveyConfig.ratingButtonColor, 'rating button color'), | ||
| submitButtonColor: validateColor(editableSurveyConfig.submitButtonColor, 'button color'), | ||
| submitButtonTextColor: validateColor(editableSurveyConfig.submitButtonTextColor, 'button text color'), | ||
| } | ||
|
|
||
| // Check if there are any validation errors | ||
| const hasErrors = Object.values(errors).some((error) => error !== undefined) | ||
| setValidationErrors(errors) | ||
|
|
||
| if (hasErrors) { | ||
| return | ||
| } | ||
|
|
||
| // If no errors, proceed with the update | ||
| updateCurrentTeam({ | ||
| survey_config: { | ||
| ...currentTeam?.survey_config, | ||
| appearance: editableSurveyConfig, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
this is the only logic changes for the update settings call
|
@marandaneto now validating on the settings page too! took a while as had to make changes to the code so it worked everywhere |
📸 UI snapshots have been updated18 snapshot changes in total. 0 added, 18 modified, 0 deleted:
Triggered by this commit. |
| @@ -31,7 +31,7 @@ | |||
| background-color: inherit; | |||
| } | |||
|
|
|||
| .Field--error &, | |||
| .Field--error &:not(.ignore-error-border), | |||
There was a problem hiding this comment.
could this cause issues since its a shared component across the app?
There was a problem hiding this comment.
I don't think so, as this class is not used anywhere else
There was a problem hiding this comment.
I see a lot of usages https://github.com/search?q=repo%3APostHog%2Fposthog+%3CLemonInput+&type=code
just double check its alright.
There was a problem hiding this comment.
yeah I meant for this class specifically ignore-error-border. And the CSS rule will still be applied normally, just to not fields that have it
| return undefined | ||
| } | ||
| // Test if the color value is valid using CSS.supports | ||
| const isValidColor = CSS.supports('color', color) |
There was a problem hiding this comment.
| const isValidColor = CSS.supports('color', color) | |
| let isValidColor = CSS.supports('color', color) | |
| if (!isValidColor) isValidColor = CSS.supports('color', `#${color}`) |
I think it's worth trying to add a # and try again if it fails, either here or elsewhere.
Attention that the given color will be mutated in this case, the caller will need to use the new value if it turns out to the a missing # issue
There was a problem hiding this comment.
created a new function to sanitize the survey's appearance, this way it'll work even without the #
Problem
adds form validation to CSS colors inside Surveys' form to prevent rendering issues
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?
Tested if validation worked as expected locally