feat: Add Embed </> options to Survey cta bar and minor changes#3272
feat: Add Embed </> options to Survey cta bar and minor changes#3272jobenjada merged 11 commits intoformbricks:mainfrom
Conversation
|
@NawinKumarSharma is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce new props, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
enhancement
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/EmbedView.tsx (2)
52-56: LGTM: Styling adjustments for back button presenceThe conditional styling for the sidebar and content area based on the
disableBackprop is well implemented. It ensures proper spacing when the back button is not present.For consistency, consider using the same condition structure for both the sidebar and content area. You could update line 77 to match the structure used in the sidebar:
- <div className={!disableBack ? "mt-10" : ""}> + <div className={cn(!disableBack && "mt-10")}>This change would make the code more uniform and easier to read.
Also applies to: 77-77
Line range hint
1-114: Summary of review for EmbedView.tsxOverall, the changes to the
EmbedViewcomponent align well with the PR objectives to rework the Survey Summary CTA bar. The addition of thedisableBackprop and the corresponding UI adjustments are good improvements. However, there are a few points to address:
- The most critical issue is the inverted logic in the back button rendering condition. This needs to be fixed to ensure correct functionality.
- For consistency, consider updating the conditional class application in the content area to match the structure used in the sidebar.
Once these points are addressed, the changes will significantly enhance the functionality and user experience of the Survey Summary CTA bar.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/PanelInfoView.tsx (1)
10-10: LGTM! Consider renaming for clarity.The addition of the
disableBackproperty to thePanelInfoViewPropsinterface is appropriate. It aligns with the component's new functionality to conditionally render a "Back" button.Consider renaming
disableBacktoshowBackButtonfor improved clarity. This would make the property's purpose more immediately obvious and align better with its usage in the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/EmbedView.tsx (4 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/PanelInfoView.tsx (1 hunks)
🔇 Additional comments (13)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/EmbedView.tsx (2)
17-17: LGTM: Addition ofdisableBackpropertyThe new
disableBackboolean property in theEmbedViewPropsinterface is a good addition. It aligns with the PR objectives to rework the Survey Summary CTA bar and provides a clear way to control the visibility of the back button.
28-28: LGTM:disableBackprop added to componentThe
disableBackprop has been correctly added to theEmbedViewcomponent's destructured props. This implementation allows the new functionality to be passed into the component as intended.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/PanelInfoView.tsx (1)
14-14: LGTM! Component signature updated correctly.The
PanelInfoViewcomponent signature has been properly updated to include the newdisableBackprop. This change correctly implements the interface modification and uses appropriate destructuring syntax.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx (5)
26-26: 'modalType' prop correctly added to 'ShareEmbedSurveyProps' interfaceThe addition of the
modalTypeprop enhances the component's flexibility by allowing different modal views.
32-39: Updated function signature includes 'modalType' propThe function now accepts the
modalTypeprop, which is essential for rendering content based on modal type.
59-59: Properly updating 'showView' state in 'handleOpenChange' functionThe
setShowViewcorrectly resets the view when the modal opens or closes.
122-122: Inconsistent 'disableBack' prop values for 'EmbedView' componentIn the
EmbedViewcomponent, thedisableBackprop is set totruewhenmodalTypeis"start"(line 122) andfalsewhenmodalTypeis"embed"(line 145). This inconsistency might lead to unexpected behavior. Please verify if this difference is intentional. If not, consider using a consistent value for thedisableBackprop.Also applies to: 145-145
145-145: Ensure consistent 'disableBack' prop in 'EmbedView' across different modal typesWhen
modalTypeis"embed", thedisableBackprop inEmbedViewis set tofalse(line 145). Confirm whether this is intentional, especially if the back functionality should be consistent regardless of how the modal is opened.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx (5)
24-30: Good practice: Using a dedicated props interfaceDefining
SurveyAnalysisCTAPropsenhances type safety and improves code readability.
50-55: Consolidated modal state managementUsing a single
modalStateobject simplifies state management for modals and improves maintainability.
57-57: Efficient use ofuseMemoforsurveyUrlMemoizing
surveyUrlwithuseMemoprevents unnecessary recalculations and enhances performance.
91-97: Well-structured modal state handlerThe
handleModalStatehigher-order function cleanly updates specific modal states, enhancing code reusability.
184-212: Effective conditional rendering of modalsConditionally rendering
ShareEmbedSurveyandSuccessMessagecomponents based onmodalStateoptimizes performance and user experience.
...nvironmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/EmbedView.tsx
Outdated
Show resolved
Hide resolved
...onmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/PanelInfoView.tsx
Outdated
Show resolved
Hide resolved
...nments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx
Outdated
Show resolved
Hide resolved
...nments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx
Outdated
Show resolved
Hide resolved
...ments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx
Show resolved
Hide resolved
nawinsharma
force-pushed
the
survey-cta-bar
branch
from
16c95f4 to
82679d1
Compare
|
@Dhruwang , please review it! |
jobenjada
left a comment
jobenjada
left a comment
There was a problem hiding this comment.
hey mate!
thanks for your submission.
There are a few issues:
The 'Back' buttons don't work here:
The button size doesn't add up:
Pls fix these before merge
| interface ShareEmbedSurveyProps { | ||
| survey: TSurvey; | ||
| open: boolean; | ||
| modalType: "start" | "embed" | "panel"; |
There was a problem hiding this comment.
pls change variable name to modalView
| )} | ||
|
|
||
| {user && <SuccessMessage environment={environment} survey={survey} />} | ||
| {user && ( |
There was a problem hiding this comment.
embedding the same modal 3x just because a prop changes is not the right approach. pls refactor :)
| Back | ||
| </Button> | ||
| </div> | ||
| {!disableBack && ( |
There was a problem hiding this comment.
look like double negation got you here ;)
nawinsharma
force-pushed
the
survey-cta-bar
branch
from
8b9d20f to
5613181
Compare
|
@jobenjada can you review it ? Screencast.from.2024-10-07.15-42-05.webm |
|
hey mate! kiiiinda works ;) 2 things: if you open the modal the second time, you end up here again: pls make your solution more robust. secondly, we need to tweak it a bit more (and I'll add 150 points for this as it wasn't part of the original ticket): we need to pass the survey type to the modal. If the survey type is not 
the other three tabs don't make any sense for web and app surveys. Thanks mate 🙌 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx (2)
45-45: LGTM: Improved props typing and state managementThe use of
SurveyAnalysisCTAPropsinterface and the consolidatedmodalStateobject are excellent improvements. TheuseMemohook for the survey URL is a good optimization.Consider adding a comment explaining the structure of the
modalStateobject for better maintainability:// modalState controls the visibility of various modals and dropdowns const [modalState, setModalState] = useState<ModalState>({ // ... (rest of the code) });Also applies to: 50-57
111-189: LGTM: Enhanced functionality with new buttons and dropdown menuThe addition of "Embed" and "Preview" buttons, along with the dropdown menu for additional actions, significantly improves the component's functionality. These changes align well with the PR objectives.
Consider adding aria-labels to the buttons for improved accessibility:
<Button variant="secondary" size="sm" onClick={() => handleModalState("embed")(true)} EndIcon={Code2Icon} aria-label="Embed survey"> Embed </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx (1 hunks)
- packages/ui/components/Button/index.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/ui/components/Button/index.tsx (1)
96-96: Improved code readability for secondary variant styling.The modification to group the class names for the enabled state of the 'secondary' variant on a single line enhances code readability. This change doesn't affect the functionality of the component but makes the styling logic more cohesive and easier to understand at a glance.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx (4)
6-7: LGTM: Improved imports and type definitionsThe new imports and interface definitions enhance the component's functionality and type safety. The
ModalStateinterface, in particular, is a good addition for managing multiple modal states in a structured manner.Also applies to: 9-10, 16-22, 24-37
69-78: LGTM: Improved modal state managementThe refactored
handleShareModalTogglefunction and the newhandleModalStatefunction provide a more flexible and maintainable approach to managing multiple modal states. This change aligns well with the newModalStatestructure.Also applies to: 98-103
80-91: LGTM: Improved clipboard handlingThe
handleCopyLinkfunction has been updated to use a promise-based approach with proper error handling. This change addresses the issue raised in a previous review comment and improves the robustness of the clipboard operation.
Line range hint
1-224: Overall LGTM: Significant improvements to SurveyAnalysisCTA componentThe changes to the SurveyAnalysisCTA component represent a substantial improvement in functionality, state management, and code organization. The new features, including the embed option, preview functionality, and additional dropdown actions, align well with the PR objectives.
Key improvements:
- Enhanced type safety with new interfaces
- Improved state management with the consolidated
modalStateobject- New functionality with embed, preview, and dropdown actions
- Better error handling for clipboard operations
While the overall implementation is solid, consider the following minor improvements:
- Add comments for better code documentation
- Enhance accessibility with aria-labels on buttons
- Refactor the multiple ShareEmbedSurvey components for better efficiency
Great work on this significant update to the component!
...ments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx
Show resolved
Hide resolved
|
@NawinKumarSharma are you willing to wrap this up for the additional 150 points mentioned? |
4 participants
What does this PR do?
This PR resolves Reworks Survey Summary CTA bar:
Fixes: #3217
before:
after changes:
Screencast.from.2024-10-03.15-51-38.webm
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation