fix: Added DialogTitle and DialogDescrtiption components to the dialog.#3146
fix: Added DialogTitle and DialogDescrtiption components to the dialog.#3146Dhruwang merged 6 commits intoformbricks:mainfrom
Conversation
This will fix the warnings and errors that appear when a any modal is opened. dialog-content final fix Dialog Description fix minor fix
|
@SaiSawant1 is attempting to deploy a commit to the formbricks Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
github-actions
bot
added
bug
SaiSawant1
changed the title
|
@Dhruwang looks good to me, pls test and add to merge queue 🤓 |
jobenjada
left a comment
jobenjada
left a comment
There was a problem hiding this comment.
to enable merge once Dhru reviewed
jobenjada
left a comment
jobenjada
left a comment
There was a problem hiding this comment.
hey @SaiSawant1
There's also this modal with the Dialog Title error (and another error and warning):
Please fix these, the rest looks good 🙏
SaiSawant1
force-pushed
the
dialog-content-fix
branch
from
3c17042 to
08616ff
Compare
|
Warning Rate limit exceeded@Dhruwang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 40 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve enhancements to dialog components across several files, focusing on adding Changes
Assessment against linked issues
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 (
|
![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: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareSurveyResults.tsx (3 hunks)
- packages/ui/Modal/index.tsx (1 hunks)
- packages/ui/ShareSurveyLink/components/SurveyLinkDisplay.tsx (1 hunks)
- packages/ui/SurveysList/components/CopySurveyForm.tsx (1 hunks)
Additional comments not posted (4)
packages/ui/ShareSurveyLink/components/SurveyLinkDisplay.tsx (1)
12-12: Consider the implications of usingdefaultValue.The change from
valuetodefaultValueallows the user to freely edit the input, which enhances user experience. However, keep in mind that any changes made by the user will not be automatically reflected in thesurveyUrlprop.If you need to keep the
surveyUrlin sync with user input, you'll need to implement additional logic to manage the state externally. Verify if this is required for your use case and make the necessary adjustments.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareSurveyResults.tsx (1)
5-5: LGTM!The import statement is correct and the imported components are used in the subsequent code changes.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareEmbedSurvey.tsx (2)
58-61: LGTM!The addition of
DialogTitleandDialogDescriptioncomponents improves the semantic structure and accessibility of the dialog. The title text is clear and appropriate. Hiding the description is a good approach if it's intended for future use or accessibility purposes without displaying additional information to the user.
10-10: LGTM!The import statement for
DialogTitleandDialogDescriptioncomponents is correctly formatted and includes the necessary components from the@formbricks/ui/Dialogmodule.
| <DialogHeader> | ||
| <DialogTitle> | ||
| <CheckCircle2Icon className="h-20 w-20 text-slate-300" /> | ||
| </DialogTitle> | ||
| </DialogHeader> | ||
| <DialogDescription className="hidden" /> |
There was a problem hiding this comment.
Add a meaningful description to DialogDescription instead of hiding it.
The addition of DialogHeader and DialogTitle improves the semantic structure and accessibility of the dialog. Wrapping the icon within DialogTitle provides a meaningful title for the dialog.
However, hiding the DialogDescription defeats its purpose. The DialogDescription is meant to provide additional context for screen readers. Instead of hiding it, consider providing a meaningful description that enhances the accessibility of the dialog.
| <DialogHeader> | ||
| <DialogTitle> | ||
| <AlertCircleIcon className="h-20 w-20 text-slate-300" /> | ||
| </DialogTitle> | ||
| </DialogHeader> | ||
| <DialogDescription className="hidden" /> |
There was a problem hiding this comment.
Add a meaningful description to DialogDescription instead of hiding it.
The addition of DialogHeader and DialogTitle improves the semantic structure and accessibility of the dialog. Wrapping the icon within DialogTitle provides a meaningful title for the dialog.
However, hiding the DialogDescription defeats its purpose. The DialogDescription is meant to provide additional context for screen readers. Instead of hiding it, consider providing a meaningful description that enhances the accessibility of the dialog.
| <DialogPrimitive.DialogTitle> | ||
| {title && <p className="mb-4 text-xl font-bold text-slate-500">{title}</p>} | ||
| </DialogPrimitive.DialogTitle> | ||
| <DialogPrimitive.DialogDescription></DialogPrimitive.DialogDescription> |
There was a problem hiding this comment.
Use semantic HTML for the dialog title and description.
Please consider the following changes:
-
Replace the
<p>tag with an appropriate heading tag (e.g.,<h2>,<h3>) for the dialog title. Using a<p>tag for the title is not semantically correct and can negatively impact SEO and accessibility. -
Either remove the empty
DialogPrimitive.DialogDescriptioncomponent or provide a meaningful description. An empty description component can confuse users of assistive technologies.
Apply this diff to address the issues:
-<DialogPrimitive.DialogTitle>
- {title && <p className="mb-4 text-xl font-bold text-slate-500">{title}</p>}
-</DialogPrimitive.DialogTitle>
-<DialogPrimitive.DialogDescription></DialogPrimitive.DialogDescription>
+<DialogPrimitive.DialogTitle>
+ {title && <h2 className="mb-4 text-xl font-bold text-slate-500">{title}</h2>}
+</DialogPrimitive.DialogTitle>
+{description && (
+ <DialogPrimitive.DialogDescription>
+ {description}
+ </DialogPrimitive.DialogDescription>
+)}Note: Replace description with the actual description content or remove the entire DialogPrimitive.DialogDescription block if no description is needed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <DialogPrimitive.DialogTitle> | |
| {title && <p className="mb-4 text-xl font-bold text-slate-500">{title}</p>} | |
| </DialogPrimitive.DialogTitle> | |
| <DialogPrimitive.DialogDescription></DialogPrimitive.DialogDescription> | |
| <DialogPrimitive.DialogTitle> | |
| {title && <h2 className="mb-4 text-xl font-bold text-slate-500">{title}</h2>} | |
| </DialogPrimitive.DialogTitle> | |
| {description && ( | |
| <DialogPrimitive.DialogDescription> | |
| {description} | |
| </DialogPrimitive.DialogDescription> | |
| )} |
| {product?.environments.map((environment, idx) => { | ||
| return ( | ||
| <FormField | ||
| key={idx} |
There was a problem hiding this comment.
Use a unique and stable identifier for the key prop.
Using the index as the key prop is not recommended, as it can lead to issues if the order of items in the product?.environments array changes. Consider using a unique and stable identifier, such as environment.id, for the key prop instead.
Apply this diff to use environment.id as the key:
-{product?.environments.map((environment, idx) => {
+{product?.environments.map((environment) => {
return (
<FormField
- key={idx}
+ key={environment.id}
control={form.control}
name={`products.${productIndex}.environments`}
render={({ field }) => {Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareSurveyResults.tsx (1)
Line range hint
24-86: Consider addingDialogTitleandDialogDescriptionto improve accessibility.The transition from
DialogtoModalcomponent aligns with the design system and may offer a better user experience. The core functionality of displaying survey results and handling publish/unpublish actions remains intact.However, the removal of
DialogContent,DialogTitle, andDialogDescriptioncomponents may impact the accessibility and semantic structure of the dialog. Consider adding a meaningfulDialogTitleandDialogDescriptionto provide context for screen readers and enhance accessibility.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareSurveyResults.tsx (4 hunks)
Additional comments not posted (1)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ShareSurveyResults.tsx (1)
5-5: LGTM!The import statement for the
Modalcomponent is correct and matches its usage in the code.
Dhruwang
left a comment
Dhruwang
left a comment
There was a problem hiding this comment.
Looks good, Thanks @SaiSawant1 😊🚀🙌🏻
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
/award 50 |
|
Awarding SaiSawant1: 50 points! Check out your new contribution on oss.gg/SaiSawant1 |
3 participants
This will fix the warnings and errors that appear when a any modal is opened.
dialog-content final fix
Dialog Description fix
minor fix
What does this PR do?
This PR will resolve all the warnings related to modal/dialog components.
Radix dialog content requires the DialogTitle to be children of the DialogContent.
Fixes #3098
before-fix.mov
after-fix.mov
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation