-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: adds image adding option to all questions #1305
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
feat: adds image adding option to all questions #1305
Conversation
|
@rtpa25 is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
The new code added in the PR is mostly sound, however there are a couple of points that could use a bit of enhancement:
Here's a way to that: import { useEffect } from "react";
...
useEffect(() => {
setShowImageUploader(!!question.imageUrl);
}, [question.imageUrl]);
<ImagePlusIcon
aria-label="Toggle image uploader"
className="ml-2 h-4 w-4 cursor-pointer text-slate-400 hover:text-slate-500"
onClick={() => setShowImageUploader((prev) => !prev)}
/>
<img src={question.imageUrl} alt={`Image for ${question.headline}`} className={"my-4 rounded-md"} />With these changes, your PR would be even more solid! Remember to test your application thoroughly after updating any dependencies to ensure that everything still works as expected. While this change itself doesn't require any code suggestions, the following might be helpful for your overall project: consider using a package like You can install it globally in the development environment and run it to update all packages at once: npm install -g npm-check-updates
ncu -u
npm installIs there any reason
- x-smtp-password: &smtp_password # Set the below value to your public-facing URL, e.g., https://example.com
+ x-smtp-password: ${SMTP_PASSWORD}
- x-smtp-port: &smtp_port # Enable SMTP_SECURE_ENABLED for TLS (port 465)
+ # Enable SMTP_SECURE_ENABLED for TLS (port 465)
+ x-smtp-port: &smtp_portOther than these points, your formatting changes help clear up some clutter, making the code more readable. Good job! import * as Joi from 'joi';
const imageUrlValidationSchema = Joi.string().uri();
const isValidUrl = (url: string) => imageUrlValidationSchema.validate(url).error ? false : true;You can use this validation function if(isValidUrl(imageUrl)) {
// perform operation
} else {
// return or throw error
}Remember, the suggestion above depends on the However, whether you need to validate the imageUrl before using it depends on the trust level of the input and the operations you will perform with it. For example, if the imageUrl is user-generated and will be rendered on client-side, validating it against XSS attacks might be important. If it's a server-internal URL, validation may not be necessary. Also, please make sure to perform thorough testing after changing dependencies' versions to assure the system's stability and performance. Here is a suggestion: ...
turbo:
specifier: latest
- version: 1.10.12
+ version: 1.10.14
...Ensure that the entire system is tested thoroughly after the changes to the dependencies are made. |
...pp/(app)/environments/[environmentId]/surveys/[surveyId]/edit/components/CTAQuestionForm.tsx
Outdated
Show resolved
Hide resolved
| "lucide-react": "^0.287.0", | ||
| "mime": "^3.0.0", | ||
| "next": "13.5.5", | ||
| "next": "13.5.6", |
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.
|
LGTM |
|
Also I think |
|
@anjy7 Not necessarily, UI components are Input, Label etc which do not have any business specific logic, but this has packages/ui is the ui library of a product, this does not belong there IMO thoughts @mattinannt |
neilchauhan2
left a 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.
@rtpa25 Thank you so much for your contribution 🎉. The functionality is working great, but I have mentioned a few issues, could you please address them. I'll review this PR again post that.
cc: @mattinannt
| updateQuestion: (questionIdx: number, updatedAttributes: any) => void; | ||
| isInValid: boolean; | ||
| environmentId: string; | ||
| ref?: RefObject<HTMLInputElement>; |
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.
Would like to know the reason behind using ref here? The browser console is throwing a warning.
| <QuestionFormInput | ||
| environmentId={environmentId} | ||
| isInValid={isInValid} | ||
| ref={questionRef} |
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.
What are we using this ref for?
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.
it routes back to the Input inside the questionforminput component as used above
| onFileUpload={(url: string) => { | ||
| updateQuestion(questionIdx, { imageUrl: url }); | ||
| }} |
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.
Can we extract out this function outside instead of writing this inline, for better readability and reusability to something like handleFileUpload
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.
Look at other places updateQuestion is being used and this #1305 (comment)
| onChange={(e) => updateQuestion(questionIdx, { headline: e.target.value })} | ||
| isInvalid={isInValid && question.headline.trim() === ""} | ||
| /> | ||
| <ImagePlusIcon | ||
| aria-label="Toggle image uploader" | ||
| className="ml-2 h-4 w-4 cursor-pointer text-slate-400 hover:text-slate-500" | ||
| onClick={() => setShowImageUploader((prev) => !prev)} |
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.
Same here can we extract out the onClick and onChange functions outside instead of inline functions?
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.
this is more readable as it's a one liner, and is repeated in a ton of places inside the codebase, creating separate handlers is unnecessary clutter and over head cause you need to give good names to them too
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img src={question.imageUrl} alt="question-image" className={"my-4 rounded-md"} /> | ||
| </div> |
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.
Could you please remove this eslint-disable comment?
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img src={question.imageUrl} alt="question-image" className={"my-4 rounded-md"} /> | ||
| </div> |
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.
Could you please resolve the eslint issue here as well?
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.
check comment below
| <div className="my-4 rounded-md"> | ||
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img src={question.imageUrl} alt="question-image" className={"my-4 rounded-md"} /> | ||
| </div> | ||
| )} |
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.
Here as well could you please resolve the eslint issue.
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 es-lint rule is wrong hence warning too, this package is written with preact, can't access next image here. Can in dev cause of turbo common node_modules, but not in prod
| turbo: | ||
| specifier: latest | ||
| version: 1.10.14 | ||
| version: 1.10.12 |
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.
Why have we downgraded the turbo version? Have we tested if it would break something?
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.
as I said below
| /next@13.5.6(react-dom@18.2.0)(react@18.2.0): | ||
| resolution: {integrity: sha512-Y2wTcTbO4WwEsVb4A8VSnOsG1I9ok+h74q0ZdxkwM3EODqrs4pasq7O0iUxbcS9VtWMicG7f3+HAj0r1+NtKSw==} | ||
| engines: {node: '>=16.14.0'} | ||
| hasBin: true | ||
| peerDependencies: | ||
| '@opentelemetry/api': ^1.1.0 | ||
| react: ^18.2.0 | ||
| react-dom: ^18.2.0 | ||
| sass: ^1.3.0 | ||
| peerDependenciesMeta: | ||
| '@opentelemetry/api': | ||
| optional: true | ||
| sass: | ||
| optional: true | ||
| dependencies: | ||
| '@next/env': 13.5.6 | ||
| '@swc/helpers': 0.5.2 | ||
| busboy: 1.6.0 | ||
| caniuse-lite: 1.0.30001547 |
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.
There are lot of changes here in the pnpm-lock.yaml file could you please mention, all the changes we are making in the different packages, and if they are going to bring some breaking changes?
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.
Devs don't change lock files, I have just upgraded the nextjs version as mentioned here #1305 (comment). These are peer deps for that, I have no control over that
|
@mattinannt done with the review, could you please look into this. |
What does this PR do?
Fixes #1258
https://www.loom.com/share/6cb2314656094e5e90f0b2d5abc71ca8
Type of change
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated