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.

Sign up for GitHub

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

Jump to bottom

Fix long description overflow #466

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Fix long description overflow #466

merged 5 commits into from
Jul 13, 2023

Conversation

niteshseram
Copy link
Contributor

@niteshseram niteshseram commented Jul 3, 2023 •
edited
Loading

What does this PR do?

The preview in Link Survey and In-App Survey was not having any scroll behavior when the description or title are too long and it overflowed. This PR fixes those issues.

Fixes #427

image
image
image

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
  • Added a screen recording or screenshots to this PR
  • Filled out the "How to test" section in this PR
  • Read the contributing guide
  • 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
  • Updated the Formbricks Docs if changes were necessary

@vercel
Copy link

vercel bot commented Jul 3, 2023

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

A member of the Team first needs to authorize it.

@niteshseram niteshseram changed the title Fix long descr Fix long description overflow Jul 3, 2023
jobenjada
jobenjada reviewed Jul 3, 2023
View reviewed changes
Copy link
Member

@jobenjada jobenjada left a comment

Choose a reason for hiding this comment

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

Hey @niteshseram

thanks a lot for your contribution :)

I left two comments in the code.

Regarding your overall solution:

We had the same problem when a Select question had too many options to choose from. We decided to add the scroll to the options instead of the overall widget.

Why did you decide to add the scroll to the complete widget instead of the description field? :)

Thanks! 🙌

apps/web/app/environments/[environmentId]/surveys/[surveyId]/edit/SurveyMenuBar.tsx Outdated
@@ -129,7 +129,7 @@ export default function SurveyMenuBar({
className="w-72 border-white hover:border-slate-200 "
/>
</div>
{localSurvey?.responseRate && (
{!!localSurvey?.responseRate&& (
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this?

I'm sceptical this "&&" operator works without the space.

And why did you invest this?

Copy link
Contributor Author

@niteshseram niteshseram Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jobenjada This is a file That I didn't touch. Not sure, how this got pushed. Will revert this

apps/web/app/s/[surveyId]/LinkSurvey.tsx Outdated Show resolved Hide resolved
@jobenjada jobenjada self-assigned this Jul 3, 2023
@niteshseram
Copy link
Contributor Author

Hey @niteshseram

thanks a lot for your contribution :)

I left two comments in the code.

Regarding your overall solution:

We had the same problem when a Select question had too many options to choose from. We decided to add the scroll to the options instead of the overall widget.

Why did you decide to add the scroll to the complete widget instead of the description field? :)

Thanks! 🙌

We can use that approach as well. But don't you think there will be too many scroll bars on one screen? Question and description doesn't limit, so there will be scroll for both again and there will be scroll for many options again.

@jobenjada
Copy link
Member

I think you're right!

I've played around with it and it makes more sense to have a global scroll vs. one for the single fields - definitely better UX, especially on smartphones.

  1. Can you update your PR so that the scrolling for MultiSelect and SingleSelect are removed in both link surveys and the JS widget?
  2. You did not yet add the scrolling to the JS widget
    grafik

For the understanding (its a bit messy but we have a plan to unify the form components, just didnt get to it yet):

The components used in the JS widget are different from the components used in the Preview and the Link Survey. This is why you have to add the scrolling to the JS widget.

Please note: The JS widget is PREACT and does not take tailwind classes well. I already designed the scroll bar for the scrolling in the Select questions so adding fb-overflow-y-auto should already do the trick ;)

Thanks!!

@niteshseram
Copy link
Contributor Author

I think you're right!

I've played around with it and it makes more sense to have a global scroll vs. one for the single fields - definitely better UX, especially on smartphones.

  1. Can you update your PR so that the scrolling for MultiSelect and SingleSelect are removed in both link surveys and the JS widget?
  2. You did not yet add the scrolling to the JS widget
    grafik

For the understanding (its a bit messy but we have a plan to unify the form components, just didnt get to it yet):

The components used in the JS widget are different from the components used in the Preview and the Link Survey. This is why you have to add the scrolling to the JS widget.

Please note: The JS widget is PREACT and does not take tailwind classes well. I already designed the scroll bar for the scrolling in the Select questions so adding fb-overflow-y-auto should already do the trick ;)

Thanks!!

Added scroll in JS Widget and remove the scroll from multiple choice questions @jobenjada
Thanks🙌

@jobenjada
Copy link
Member

Hey Nitesh,

here are the results of my testing:

  1. Link Survey not centered anymore:
    grafik

When you make your changes, please also test how the surveys behave when they are embedded by our iframe:
grafik

  1. There is no margin-top in the JS widget:
    grafik

Other than that, it looks great :)

@niteshseram
Copy link
Contributor Author

Hi @jobenjada, Fixed the above-mentioned issues. Thanks

@jobenjada
Copy link
Member

@mattinannt ready to merge 🚀

mattinannt
mattinannt approved these changes Jul 13, 2023
View reviewed changes
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.

@niteshseram thanks a lot for this fix 🎉💪 Let's merge it into main 😊

@mattinannt mattinannt merged commit 8ae8afe into formbricks:main Jul 13, 2023
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
@niteshseram @jobenjada
Fix long description overflow in questions (formbricks#466)
a79a8b0 
* Fix issue 427

* Address review comment

* jump back to top on long questions

* add to JS widget

---------

Co-authored-by: Seram Nitesh Singh <nitesh.s@auzmor.com>
Co-authored-by: Johannes <johannes@formbricks.com>
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
@niteshseram @jobenjada
Fix long description overflow in questions (formbricks#466)
48e97b3 
* Fix issue 427

* Address review comment

* jump back to top on long questions

* add to JS widget

---------

Co-authored-by: Seram Nitesh Singh <nitesh.s@auzmor.com>
Co-authored-by: Johannes <johannes@formbricks.com>
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
@niteshseram @jobenjada
Fix long description overflow in questions (formbricks#466)
81eaff4 
* Fix issue 427

* Address review comment

* jump back to top on long questions

* add to JS widget

---------

Co-authored-by: Seram Nitesh Singh <nitesh.s@auzmor.com>
Co-authored-by: Johannes <johannes@formbricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jobenjada jobenjada jobenjada left review comments

@mattinannt mattinannt mattinannt approved these changes

Assignees

@jobenjada jobenjada

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to see Title in a form with long description
3 participants
@niteshseram @jobenjada @mattinannt