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: Onboarding revamp #2073

Merged
merged 55 commits into from
Feb 28, 2024
Merged

feat: Onboarding revamp #2073

merged 55 commits into from
Feb 28, 2024

Conversation

Dhruwang
Copy link
Contributor

What does this PR do?

Fixes 1666

How should this be tested?

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

Copy link

vercel bot commented Feb 14, 2024

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 Feb 28, 2024 5:04pm
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2024 5:04pm

Copy link
Contributor

github-actions bot commented Feb 14, 2024

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

Copy link
Contributor

packages/ui/Onboarding/components/InviteTeamMate.tsx

It's a good practice to not expose the actual error message to the end user. Instead, log the error message for debugging and show a generic error message to the user.
Create Issue
See the diff
Checkout the fix

    try {
      // code
    } catch (error) {
      console.error(error);
      toast.error("An unexpected error occurred");
    }
git fetch origin && git checkout -b ReviewBot/Impro-jmc1dm6 origin/ReviewBot/Impro-jmc1dm6

packages/ui/Onboarding/components/Connect.tsx

Instead of using the visibilitychange event to fetch the environment every time the document becomes visible, consider using a more efficient method such as polling or websockets to keep the environment data up-to-date.
Create Issue
See the diff
Checkout the fix

    // code
git fetch origin && git checkout -b ReviewBot/Impro-2fwlzvc origin/ReviewBot/Impro-2fwlzvc

Consider using a more descriptive name for the setloading function. A name like setLoadingState would be more descriptive and improve the readability of the code.
Create Issue
See the diff
Checkout the fix

    const [loading, setLoadingState] = useState(false);
git fetch origin && git checkout -b ReviewBot/Impro-uivhwqy origin/ReviewBot/Impro-uivhwqy

packages/ui/Onboarding/components/templates.ts

  There is a repeated pattern in the code where a question object is created with a specific structure. This pattern can be extracted into a function to improve code readability and maintainability.

Create Issue
See the diff
Checkout the fix

    function createQuestion(id: string, type: TSurveyQuestionType, headline: string, required: boolean, subheader: string = '', inputType: string = 'text'): QuestionType {
      return {
        id,
        type,
        headline,
        required,
        subheader,
        inputType,
      };
    }
git fetch origin && git checkout -b ReviewBot/Impro-l1c037c origin/ReviewBot/Impro-l1c037c
  The code contains several string literals that are used multiple times. These can be extracted into constants to improve code readability and maintainability.

Create Issue
See the diff
Checkout the fix

    const BUG_REPORT = 'Bug report 🐞';
    const FEATURE_REQUEST = 'Feature Request 💡';
    const SUBMITTED = 'submitted';
    const EQUALS = 'equals';
    const END = 'end';
git fetch origin && git checkout -b ReviewBot/Impro-h9m6vke origin/ReviewBot/Impro-h9m6vke

apps/web/playwright/survey.spec.ts

There is a lot of repetition in the test code where similar actions are performed for different types of questions. This can be improved by creating a helper function that performs these actions and takes the necessary parameters as arguments. This will make the code more readable and easier to maintain.
Create Issue
See the diff
Checkout the fix

    async function createQuestion(page, questionType, questionDetails) {
      await page
        .locator("div")
        .filter({ hasText: new RegExp(`^${addQuestion}$`) })
        .nth(1)
        .click();
      await page.getByRole("button", { name: questionType }).click();
      await page.getByLabel("Question").fill(questionDetails.question);
      // ... other common actions
    }
git fetch origin && git checkout -b ReviewBot/Impro-kbp3fx8 origin/ReviewBot/Impro-kbp3fx8

Comment on lines 34 to 47
try {
if (!isValidEmail(email)) {
toast.error("Invalid Email");
return;
}
await inviteTeamMateAction(team.id, email, "developer", inviteMessage);
toast.success("Invite sent successful");
} catch (error) {
if (error instanceof Error) {
toast.error(error.message);
} else {
toast.error("An unexpected error occurred");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message is now logged for debugging and a generic error message is shown to the user.

Suggested change
try {
if (!isValidEmail(email)) {
toast.error("Invalid Email");
return;
}
await inviteTeamMateAction(team.id, email, "developer", inviteMessage);
toast.success("Invite sent successful");
} catch (error) {
if (error instanceof Error) {
toast.error(error.message);
} else {
toast.error("An unexpected error occurred");
}
}
try {
if (!isValidEmail(email)) {
toast.error("Invalid Email");
return;
}
await inviteTeamMateAction(team.id, email, "developer", inviteMessage);
toast.success("Invite sent successful");
} catch (error) {
console.error(error);
toast.error("An unexpected error occurred");
}

Comment on lines 22 to 34
useEffect(() => {
const handleVisibilityChange = async () => {
if (document.visibilityState === "visible") {
const refetchedEnvironment = await fetchEnvironment(environment.id);
if (!refetchedEnvironment) return;
setLocalEnvironment(refetchedEnvironment);
}
};
document.addEventListener("visibilitychange", handleVisibilityChange);
return () => {
document.removeEventListener("visibilitychange", handleVisibilityChange);
};
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace the visibilitychange event with a more efficient method such as polling or websockets to keep the environment data up-to-date.

Suggested change
useEffect(() => {
const handleVisibilityChange = async () => {
if (document.visibilityState === "visible") {
const refetchedEnvironment = await fetchEnvironment(environment.id);
if (!refetchedEnvironment) return;
setLocalEnvironment(refetchedEnvironment);
}
};
document.addEventListener("visibilitychange", handleVisibilityChange);
return () => {
document.removeEventListener("visibilitychange", handleVisibilityChange);
};
}, []);
useEffect(() => {
const interval = setInterval(async () => {
const refetchedEnvironment = await fetchEnvironment(environment.id);
if (!refetchedEnvironment) return;
setLocalEnvironment(refetchedEnvironment);
}, 5000);
return () => {
clearInterval(interval);
};
}, []);


export function Connect({ environment, webAppUrl }: { environment: TEnvironment; webAppUrl: string }) {
const router = useRouter();
const [loading, setloading] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed the 'setloading' function to 'setLoadingState' to improve code readability.

Suggested change
const [loading, setloading] = useState(false);
const [loading, setLoadingState] = useState(false);

@@ -1,7 +1,7 @@
import { surveys, users } from "@/playwright/utils/mock";
Copy link
Contributor

Choose a reason for hiding this comment

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

The helper function 'createQuestion' is created to reduce the repetition of similar actions performed for different types of questions. This function takes 'page', 'questionType', and 'questionDetails' as parameters and performs the common actions.

Suggested change
import { surveys, users } from "@/playwright/utils/mock";
async function createQuestion(page, questionType, questionDetails) {
await page
.locator("div")
.filter({ hasText: new RegExp(`^${addQuestion}`) })
.nth(1)
.click();
await page.getByRole("button", { name: questionType }).click();
await page.getByLabel("Question").fill(questionDetails.question);
// ... other common actions
}

@Dhruwang
Copy link
Contributor Author

Need to merge #2074 before testing this

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 dhru! Thanks for shipping this :)

A few notes regarding code organisation:

  1. In the UI folder we only add components which we use in several locations within Formbricks. You have added all of the Onboarding specific components to the UI package. This creates quite some inconsistency, because you added Onboarding specific (hard-coded) content like in the PathwaySelect to the UI package.

Please make sure to separate and reorganize your code as follows:

UI Package: Everything we will likely reuse in other parts of Formbricks (like the new card) add here. Make sure to follow the current setup with having a folder which includes only the index.tsx

Component Folder within Onboarding: Keep all components you only need in the onboarding > components folder within the Formbricks app.

Please refactor this PR also taking into account the feedback I shared on Slack.

I'll push my half-finished refactored PathwaySelect.tsx where the separation of concerns due to the current code organisation (mixing reusable components with specific content).

Please mark this as ready for review once your done and ping me 🤓

</div>
</div>
<div
className="flex h-96 w-80 flex-col items-center justify-center rounded-2xl border border-slate-300 bg-white p-3 shadow-lg transition ease-in-out hover:scale-105"
Copy link
Member

@jobenjada jobenjada Feb 14, 2024

Choose a reason for hiding this comment

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

card code redundant, please refactor. we will use this kind of card a lot more going forward, pls add to UI package.

I've done it bc I was getting tired of making all changes on the cards twice.

Asking GPT to "refactor folllwing all major coding best practices" led to quite a good outcome:

  setselectedPathway: (pathway: "link" | "in-app" | null) => void;
}

// Define a type for the pathway options to ensure type safety and reusability
type PathwayOptionType = "link" | "in-app";

interface PathwayOptionProps {
  title: string;
  description: string;
  onSelect: () => void;
}

// Reusable PathwayOption component for each option
const PathwayOption: React.FC<PathwayOptionProps> = ({ title, description, onSelect }) => (
  <div
    className="flex h-96 w-80 cursor-pointer flex-col items-center justify-center rounded-2xl border border-slate-300 bg-white p-3 shadow-lg transition ease-in-out hover:scale-105"
    onClick={onSelect}
    role="button" // Improve accessibility
    tabIndex={0} // Make it focusable
  >
    <div className="h-full w-full rounded-xl bg-gray-500">Image</div>
    <div className="my-4 space-y-2">
      <p className="text-xl font-medium text-slate-800">{title}</p>
      <p className="text-sm text-slate-500">{description}</p>
    </div>
  </div>
);

export default function PathwaySelect({ setselectedPathway }: PathwaySelectProps) {
  // Helper function to handle selection
  const handleSelect = (pathway: PathwayOptionType) => {
    localStorage.setItem("isNewUser", "true");
    setselectedPathway(pathway);
  };

  return (
    <div className="space-y-16 text-center">
      <div className="space-y-4">
        <p className="text-4xl font-medium text-slate-800">How would you like to start?</p>
        <p className="text-sm text-slate-500">You can always use both types of surveys.</p>
      </div>
      <div className="flex space-x-8">
        <PathwayOption
          title="Link Surveys"
          description="Create a new survey and share a link."
          onSelect={() => handleSelect("link")}
        />
        <PathwayOption
          title="In-app Surveys"
          description="Run a survey on a website or in-app."
          onSelect={() => handleSelect("in-app")}
        />
      </div>
    </div>
  );
}

@jobenjada jobenjada marked this pull request as draft February 14, 2024 13:53
@jobenjada
Copy link
Member

Additional feedback as requested:

  1. If you create a survey via Onboarding, this "Delete" button doesnt delete it anymore (might not be related):
image
  1. Between the Onboarding and the Onboarding Survey we see two different skeleton loading views, which makes it pretty ugly. Instead, can we fade out and in ( Dru I tagged you in the corresponding ticket but we need Mattis input first).

  2. This needs to be smoother. We shouldn't have the button nor the countdown. I'll have a think how we can get a smoother transition back into the app. Ideal would be a fade:

image

(btw the "Finish Onboarding" showed me to the login page loaded within the onboarding?)

image

I also plan to remove the Formbricks branding, doesn't make sense on our own Onboarding survey.

  1. Here I had to click twice on the card in the first few times:
image
  1. Can we keep this on the page.tsx so that it doesn't reload when choosing the pathway? Generally, do we need a route for each step?
image

Here please reuse the UI from the previous onboarding, feels much more native. Augment it to have the "Other" option working:
image

This also resolves the transition / forwarding problem.

  1. Is "See example survey in your app" button working? It didn't work for me:
image image

I don't think it creates a survey, at least I'm seeing the Onboarding modal for Link Surveys (and no survey in the list):

image image
  1. Redirect loop: Not entirely sure when it happened but I had already received a signal and set Onboarding to false via Prisma to walk through the onboarding again. At the end I got stuck in the loop:
image
  1. Dead button
image
  1. Handle bar not replace {Full Name}
image
  1. After the coworker was invited we should be forwarded to explore Formbricks:
image

Here we won't show the modal with 3 templates because we already have this page:

image

That's it for now, thanks :)

@jobenjada
Copy link
Member

Hey! Here is my feedback:

Link Survey Pathway:

  1. Survey not found:
image

In App Pathway:

  1. Other field does not allow additional text input. It's supposed to function like the "Other" field in our surveys:
image
  1. Leave out step 3: This we don't need
image
  1. Between the different routes, we reload the top navigation (logo and progress bar). That doesn't look or feel good, it breaks the Onboarding smootheness and creates an impression of Formbricks as a hacked together product. Since the Onboarding is the first impression, it has to look, work and feel smooth.

The top bar flashes up between:

  1. First select (pathway -> in app onboarding survey)
  2. Onboarding Survey and the Connect screens:
image

Why don't you keep the Logo and Progress bar in a layout.tsx and just update the Progress bar value? Then it should get reloaded. Generally, I'm not sure if we need a new route for each step. Why did you decide to build it like that?


  1. NPM button is gone. I can add it later.

  1. No default survey displayed after I successfully connected my app with Formbricks:
image

That's what we talked about in Slack. Let's huddle tomorrow to resolve anything unclear.


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.

almost there :)

{renderOnboardingStep()}
{iframeVisible && (
<iframe
src="http://localhost:3000/s/clsui9a7x0000fbh5orp0g7c5"
Copy link
Member

Choose a reason for hiding this comment

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

@mattinannt you need to create a survey with an animation or image bg locally and replace this URL to test the Link Survey onboarding path.

Before merging into prod, you need to add the link to the Onboarding Survey we have in the Formbricks Cloud account.

Now that I think about it, we should make use of the user identification in link surveys via URL to attribute the responses to the user who just signed up. We have the user email in the session object so it shouldn't be an issue. Let me try that first!

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.

@Dhruwang Onboarding looks great! :-) On the code side there are a few smaller things I would like to see changed for more consistency 😊
Can you please also solve the merge conflicts? Then we should be ready to merge this 😊💪

apps/web/app/(app)/onboarding/components/onboarding.tsx Outdated Show resolved Hide resolved
apps/web/app/(app)/onboarding/components/onboarding.tsx Outdated Show resolved Hide resolved
@@ -29,6 +29,12 @@ export default function SurveysList({
}: SurveysListProps) {
const [filteredSurveys, setFilteredSurveys] = useState<TSurvey[]>(surveys);
const [orientation, setOrientation] = useState("grid");

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we also call this at the end of the onboarding to remove them once instead of accessing the local storage on every survey list page call?

apps/web/app/(app)/onboarding/components/onboarding.tsx Outdated Show resolved Hide resolved

import { ConnectWithFormbricks } from "@/app/(app)/onboarding/components/inapp/ConnectWithFormbricks";
import { InviteTeamMate } from "@/app/(app)/onboarding/components/inapp/InviteTeamMate";
import Objective from "@/app/(app)/onboarding/components/inapp/SurveyObjective";
Copy link
Member

Choose a reason for hiding this comment

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

Please use module exports instead of default exports:

import { Objective } from "@/app/(app)/onboarding/components/inapp/SurveyObjective";

html: "You're all set up. Create your own survey to gather exactly the feedback you need :)",
buttonLabel: "Create survey",
buttonExternal: true,
buttonUrl: "https://app.formbricks.com",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be dynamic for self hosters and should have the WEBAPP_URL instead of app.formbricks.com

@jobenjada jobenjada added this pull request to the merge queue Feb 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 28, 2024
@jobenjada jobenjada merged commit a8563ad into main Feb 28, 2024
14 checks passed
@jobenjada jobenjada deleted the onboarding-revamp branch February 28, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants