Skip to content

Conversation

@rtpa25
Copy link
Contributor

@rtpa25 rtpa25 commented Oct 19, 2023

What does this PR do?

Fixes #1258

https://www.loom.com/share/6cb2314656094e5e90f0b2d5abc71ca8

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change adds a new database migration
  • This change requires a documentation update

How should this be tested?

  • StoryBook tests

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

@rtpa25 rtpa25 marked this pull request as draft October 19, 2023 01:31
@vercel
Copy link

vercel bot commented Oct 19, 2023

@rtpa25 is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

Thank you for following the naming conventions for pull request titles! 🙏

@github-actions github-actions bot added enhancement New feature or request formtribe-2023 hacktoberfest complete these issues to gather points for Hacktoberfest labels Oct 19, 2023
@review-agent-prime
Copy link
Contributor

The new code added in the PR is mostly sound, however there are a couple of points that could use a bit of enhancement:

  1. In the OpenQuestionForm component, you are using a useState hook to initialize the showImageUploader state with logic based on whether question.imageUrl exists. A more robust approach would be to handle this logic in useEffect which runs after each render. When the question changes, the visibility of the image uploader could be set accordingly.

Here's a way to that:

import { useEffect } from "react";

...

useEffect(() => {
  setShowImageUploader(!!question.imageUrl);
}, [question.imageUrl]);
  1. Accessing the environmentId by splitting and indexing the pathname could introduce bugs if the pathname structure changes in the future. If possible, this value should be passed as a prop to the component, or obtained through a more reliable method.

  2. When adding an ImagePlusIcon onClick event, it is considered a good practice to provide an accessible name for this clickable icon using aria-label attribute.

<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)}
/>
  1. In the rendering of the image in OpenTextQuestion.tsx, it is better to provide a more informative alt text. You can use the question headline for this purpose.
<img src={question.imageUrl} alt={`Image for ${question.headline}`} className={"my-4 rounded-md"} />

With these changes, your PR would be even more solid!
The PR shows that the version of next in package.json has been updated. It's a good practice to keep packages updated, especially when the updates include important bug fixes or improvements that your project could benefit from.

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 npm-check-updates to streamline the process of keeping your dependencies up to date.

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 install

Is there any reason next was updated individually rather than together with other packages that might also need updates? Please keep updates systematic to ensure smooth dependencies handling.
The changes in this PR look good for the most part. However, let's take into consideration a few suggestions to enhance your docker-compose code:

  1. It seems you're leaving a number of settings open in your docker-compose file. It's a good practice to avoid exposing sensitive information. Ideally, sensitive data should be fetched from environment variables or secrets. For example, for x-smtp-password: &smtp_password, the SMTP password could be read from an environment variable:
-  x-smtp-password: &smtp_password # Set the below value to your public-facing URL, e.g., https://example.com
+  x-smtp-password: ${SMTP_PASSWORD}
  1. The format of comments within the yml file has been changed. Keep in mind that comments should ideally go on a separate line instead of in-line with the code to improve readability:
-  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_port

Other than these points, your formatting changes help clear up some clutter, making the code more readable. Good job!
The new property imageUrl seems to be well integrated into the ZSurveyOpenTextQuestion schema. Just make sure that it is validated in an appropriate stage before performing any critical operations. Here is a suggestion of how it can be done:

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 isValidUrl before saving or using the image URL. For instance:

if(isValidUrl(imageUrl)) {
  // perform operation
} else {
  // return or throw error
}

Remember, the suggestion above depends on the joi package. Make sure it's included in the dependencies of your project before using it.

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.
From your PR, it looks like some packages have been downgraded, for example, the version of turbo was changed from 1.10.14 to 1.10.12. It's usually recommended to use the latest stable version of the dependencies, unless there is a specific requirement to downgrade to an older version.

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.

@rtpa25 rtpa25 changed the title feat: adds image to open text question feat: adds image adding option to all questions Oct 19, 2023
"lucide-react": "^0.287.0",
"mime": "^3.0.0",
"next": "13.5.5",
"next": "13.5.6",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtpa25
Copy link
Contributor Author

rtpa25 commented Oct 19, 2023

LGTM

@rtpa25 rtpa25 marked this pull request as ready for review October 19, 2023 04:06
@anjy7
Copy link
Contributor

anjy7 commented Oct 19, 2023

Also I think packages/ui would be a better place to keep the new component, since we keep all our UI components over there @rtpa25

@rtpa25
Copy link
Contributor Author

rtpa25 commented Oct 19, 2023

@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

Copy link
Contributor

@neilchauhan2 neilchauhan2 left a 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>;
Copy link
Contributor

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}
Copy link
Contributor

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?

Copy link
Contributor Author

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

Comment on lines +37 to +39
onFileUpload={(url: string) => {
updateQuestion(questionIdx, { imageUrl: url });
}}
Copy link
Contributor

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

Copy link
Contributor Author

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)

Comment on lines +50 to +56
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)}
Copy link
Contributor

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?

Copy link
Contributor Author

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

Comment on lines +31 to +33
{/* eslint-disable-next-line @next/next/no-img-element */}
<img src={question.imageUrl} alt="question-image" className={"my-4 rounded-md"} />
</div>
Copy link
Contributor

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?

Comment on lines +33 to +35
{/* eslint-disable-next-line @next/next/no-img-element */}
<img src={question.imageUrl} alt="question-image" className={"my-4 rounded-md"} />
</div>
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check comment below

Comment on lines +97 to +101
<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>
)}
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I said below

Comment on lines +17789 to +17807
/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
Copy link
Contributor

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?

Copy link
Contributor Author

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

@rtpa25 rtpa25 requested a review from neilchauhan2 October 19, 2023 13:36
@neilchauhan2
Copy link
Contributor

@mattinannt done with the review, could you please look into this.

@mattinannt mattinannt enabled auto-merge October 20, 2023 10:42
@mattinannt mattinannt added this pull request to the merge queue Oct 20, 2023
Merged via the queue into formbricks:main with commit c2c27a7 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request hacktoberfest complete these issues to gather points for Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE][FormTribe 🔥][750 Points] Add Image to every question

4 participants