Skip to content
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: Added Dynamic Link Previews #1093

Merged
merged 27 commits into from
Oct 23, 2023
Merged

Conversation

anjy7
Copy link
Contributor

@anjy7 anjy7 commented Oct 11, 2023

What does this PR do?

Adds Dynamic Link Previews

Fixes #1064

Screenshot from 2023-10-11 21-07-22

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?

  • Share survey link

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

@vercel
Copy link

vercel bot commented Oct 11, 2023

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

A member of the Team first needs to authorize it.

@github-actions github-actions bot added enhancement New feature or request formtribe-2023 Issues related to Formtribe 2023 hackathon hacktoberfest complete these issues to gather points for Hacktoberfest labels Oct 11, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2023

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

@jobenjada
Copy link
Member

@anjy7 can you add a screenshot or recording for reference? :)

@jobenjada jobenjada closed this Oct 11, 2023
@jobenjada jobenjada reopened this Oct 11, 2023
@anjy7
Copy link
Contributor Author

anjy7 commented Oct 11, 2023

@anjy7 can you add a screenshot or recording for reference? :)

Yeah its a work in progress, will add soon. Working on the the image..

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 11, 2023

Hey @jobenjada, its ready, please have a look. The brand color and the survey name is dynamic..

@anjy7 anjy7 marked this pull request as ready for review October 11, 2023 15:39
@jobenjada jobenjada self-assigned this Oct 13, 2023
@jobenjada
Copy link
Member

jobenjada commented Oct 14, 2023

hey hey! I tried two different ways to test this and it doesnt work:

image image

And Im getting this error:
image

Is next/head still the right approach for app dir? 🤔

Could you maybe create a Loom for me to better understand how you are testing this ?

Additionally, please resolve the merge conflict :)

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 14, 2023

hey hey! I tried two different ways to test this and it doesnt work:

image image
And Im getting this error: image

Is next/head still the right approach for app dir? 🤔

Could you maybe create a Loom for me to better understand how you are testing this ?

Additionally, please resolve the merge conflict :)

sure, I'll look into this and get back to you @jobenjada

@jobenjada
Copy link
Member

@anjy7 perfect :) I know it'll be Monday morning, but can you maybe have a look tomorrow? :D Would love to include this in our Week 2 Review which I'll record tomorrow afternoon (would be evening in India)

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 15, 2023

Hey @jobenjada, trying to debug it at present. And sure I’ll try to do it before evening :)

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 16, 2023

Hey @jobenjada , I have made the changes, please have a look :)
Here's the loom recording as well - https://www.loom.com/share/35345359f50d4e219f804bb47a4d20c8?sid=dd014683-c1da-48b5-893e-15d490faac54

apps/web/app/s/[surveyId]/layout.tsx Outdated Show resolved Hide resolved
apps/web/app/s/[surveyId]/layout.tsx Outdated Show resolved Hide resolved
apps/web/app/s/[surveyId]/page.tsx Outdated Show resolved Hide resolved
"@t3-oss/env-nextjs": "^0.7.1",

Copy link
Member

Choose a reason for hiding this comment

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

please remove the empty line

Copy link
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@anjy7 thanks a lot for the PR, looks great! From the technical side I would like to see a few more things changed before we merge the changes.

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 21, 2023

Hey @mattinannt, I have made the changes. Also the share button wasn't working on the survey as ig someone had put a check whether the survey is single use or not ( this prevented the share dropdown opening if the survey is not single use ). I have removed it, please check it once :)

@mattinannt
Copy link
Member

@anjy7 thanks a lot for the changes and also fixing the bug on the way :-) 💪 I will merge the PR now :-)

@mattinannt mattinannt added this pull request to the merge queue Oct 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2023
@mattinannt
Copy link
Member

@anjy7 Unfortunately there is still a build error in the PR:

@formbricks/web:build: app/api/v1/og/route.tsx
@formbricks/web:build: Type error: Route "app/api/v1/og/route.tsx" does not match the required types of a Next.js Route.
@formbricks/web:build:   Invalid configuration "fetchCache":
@formbricks/web:build:     Expected ""auto" | "force-cache" | "force-no-store" | "only-no-store" | "default-no-store" | "default-cache" | "only-cache" | undefined", got "false".

Can you please solve this? :-) Then we are ready to merge! :-)

@mattinannt mattinannt added this pull request to the merge queue Oct 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2023
@anjy7
Copy link
Contributor Author

anjy7 commented Oct 22, 2023

Hey @mattinannt, I’ll look into this in a bit

@anjy7
Copy link
Contributor Author

anjy7 commented Oct 22, 2023

Hi @mattinannt, I built it locally and all the test cases passed..
this was the error message in the above failed merge - ../../packages/lib/storage/service.ts:70:26 Type error: Property 'getType' does not exist on type 'typeof import("/home/runner/work/formbricks/formbricks/node_modules/@types/mime/index")'.
Although it shouldnt be there as the build is successful locally, could you please maybe try merging it again?

@mattinannt mattinannt added this pull request to the merge queue Oct 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2023
@anjy7
Copy link
Contributor Author

anjy7 commented Oct 22, 2023

should work now @mattinannt
image

@mattinannt mattinannt added this pull request to the merge queue Oct 23, 2023
Merged via the queue into formbricks:main with commit 502610f Oct 23, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request formtribe-2023 Issues related to Formtribe 2023 hackathon hacktoberfest complete these issues to gather points for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE][FormTribe 🔥][500 Points] Dynamic Link Previews
3 participants