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: Pagination for other values in multi choice #1560

Merged
merged 5 commits into from
Nov 6, 2023
Merged

Conversation

Dhruwang
Copy link
Contributor

@Dhruwang Dhruwang commented Nov 2, 2023

What does this PR do?

Fixes 1466

Screen.Recording.2023-11-02.at.6.25.41.PM.mov

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?

Check summary for multi choice with other enabled

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
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 🙏

Appreciated

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

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
formbricks-cloud ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2023 9:01am
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2023 9:01am

Copy link
Contributor

github-actions bot commented Nov 2, 2023

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

Copy link
Contributor

Multiple files

The new constant name 'TEXT_RESPONSES_PER_PAGE' is more generic and improves the readability of the code. However, it's important to ensure that this change doesn't affect the functionality of the code, especially if there are other types of responses that should not be affected by this constant.
Create Issue

    // No changes are needed as the new constant name improves readability

apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/OpenTextSummary.tsx

It's recommended to use the 'useReducer' hook instead of 'useState' when the new state depends on the previous one. This is because 'useState' may not immediately update the state and this could lead to stale state issues. 'useReducer' provides a more predictable state transition.
Create Issue

    const [displayCount, setDisplayCount] = useReducer((prevCount, newCount) => prevCount + newCount, responsesPerPage);

MultipleChoiceSummary.tsx

Instead of using the filter method to remove empty values and then using slice to limit the number of displayed responses, you can use the reduce method to achieve the same result in a single iteration. This will improve the performance of your code, especially when dealing with large arrays.
Create Issue

    result.otherValues.reduce((acc, otherValue, idx) => {
      if (otherValue !== "" && acc.length < otherDisplayCount) {
        acc.push(otherValue);
      }
      return acc;
    }, [])

Instead of using an if-else statement to determine the text to display based on the count, you can use a ternary operator. This will make your code more concise and easier to read.
Create Issue

    <p className="flex w-full pt-1 text-slate-600 sm:items-end sm:justify-end sm:pt-0">
      {result.count} {result.count === 1 ? "response" : "responses"}
    </p>

packages/lib/constants.ts

Instead of calculating the file size limits each time they are accessed, you can calculate them once and store the results in constants. This will slightly improve the performance of your code.
Create Issue

    const MB = 1024 * 1024;
    const GB = MB * 1024;

    export const MAX_SIZES = {
      public: MB * 10, // 10MB
      free: MB * 10, // 10MB
      pro: GB, // 1GB
    } as const;

Consider renaming the constant TEXT_RESPONSES_PER_PAGE to TEXT_RESPONSES_LIMIT_PER_PAGE or MAX_TEXT_RESPONSES_PER_PAGE to make it more descriptive and improve code readability.
Create Issue

    export const MAX_TEXT_RESPONSES_PER_PAGE = 5;

@Dhruwang Dhruwang changed the title enhancement: Pagination for other values in multi choice feat: Pagination for other values in multi choice Nov 2, 2023
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.

🚀

@mattinannt mattinannt self-assigned this Nov 6, 2023
@mattinannt mattinannt added this pull request to the merge queue Nov 6, 2023
Merged via the queue into main with commit 46f4222 Nov 6, 2023
9 of 10 checks passed
@mattinannt mattinannt deleted the pagination branch November 6, 2023 09:02
kevinkong91 added a commit to kevinkong91/formbricks that referenced this pull request Nov 7, 2023
* main: (42 commits)
  chore: add revert to oss-friends (formbricks#745)
  fix: pass authOptions to getServerSession in authLayout (formbricks#1584)
  fix: fixed URL example for "Create Survey" API endpoint is wrong formbricks#1555 (formbricks#1586)
  fix(docs): default account info docs changes (formbricks#1583)
  chore: Add docker packages to Github Packages on release (formbricks#1585)
  feat: avatar upload (formbricks#1546)
  fix: editor crashing (formbricks#1582)
  fix: Add scroll to the setting navbar  (formbricks#1398)
  are you sure modal missing fix:formbricks#1419 (formbricks#1485)
  fix: github linting issues (formbricks#1510)
  refactor: added authorization to airtable integration and create a common actions file (formbricks#1538)
  docs: add docs for airtable (formbricks#1541)
  fix: missing static generation store in revalidation due to pages dir (formbricks#1581)
  feat: Pagination for other values in multi choice (formbricks#1560)
  feat:  $199 pricing model for unlimited plans (formbricks#1564)
  fix: make pricing pages consistent (formbricks#1567)
  fix: openText issue (formbricks#1579)
  fix: avoid blocking the request if it does not have an associated ip (formbricks#1540)
  feat: Add Unkey to OSS Friends (formbricks#1574)
  feat: FOR-683 Role Switch (formbricks#1450)
  ...
kevinkong91 added a commit to kevinkong91/formbricks that referenced this pull request Nov 7, 2023
* main: (70 commits)
  chore: add revert to oss-friends (formbricks#745)
  fix: pass authOptions to getServerSession in authLayout (formbricks#1584)
  fix: fixed URL example for "Create Survey" API endpoint is wrong formbricks#1555 (formbricks#1586)
  fix(docs): default account info docs changes (formbricks#1583)
  chore: Add docker packages to Github Packages on release (formbricks#1585)
  feat: avatar upload (formbricks#1546)
  fix: editor crashing (formbricks#1582)
  fix: Add scroll to the setting navbar  (formbricks#1398)
  are you sure modal missing fix:formbricks#1419 (formbricks#1485)
  fix: github linting issues (formbricks#1510)
  refactor: added authorization to airtable integration and create a common actions file (formbricks#1538)
  docs: add docs for airtable (formbricks#1541)
  fix: missing static generation store in revalidation due to pages dir (formbricks#1581)
  feat: Pagination for other values in multi choice (formbricks#1560)
  feat:  $199 pricing model for unlimited plans (formbricks#1564)
  fix: make pricing pages consistent (formbricks#1567)
  fix: openText issue (formbricks#1579)
  fix: avoid blocking the request if it does not have an associated ip (formbricks#1540)
  feat: Add Unkey to OSS Friends (formbricks#1574)
  feat: FOR-683 Role Switch (formbricks#1450)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants