-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add configurable tooltips #44
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
base: develop
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/components/feedback/index.tsx
Outdated
| <ReportTitleInput | ||
| title={feedbackForm.reportTitle} | ||
| onChange={feedbackForm.setReportTitle} | ||
| label={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MDN and WCAG guidelines recommend keeping labels simple and text-only. It isn't advised to insert interactive elements as labels. The best approach is the one you used here:
{/* Contexto da Atividade */}
<div className="space-y-1">
<div className="flex items-center gap-2">
<p className="text-gray-400 font-medium text-sm">Contexto da Atividade</p>
<Tooltip content={TOOLTIP_CONTENT.ai.Contexto_da_atividade} />
</div>
<textarea
value={feedbackForm.activityContext}
onChange={(e: ChangeEvent<HTMLTextAreaElement>) => feedbackForm.setActivityContext(e.target.value)}
className="w-full bg-gray-700 border border-gray-600 rounded-xl p-4 text-gray-300 text-xs leading-relaxed focus:outline-none focus:border-indigo-500 focus:ring-1 focus:ring-indigo-500 min-h-[150px] resize-y"
placeholder="Descreva o contexto da atividade..."
/>
</div>Also note that you're repeating yourself with the wrapper.
| interface ToggleSwitchProps { | ||
| id: string; | ||
| label: string; | ||
| label: ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Labels should be strings. Maybe introduce a new prop tooltipText? : string in which you can consistently check inside implementations as {tooltip && <Tooltip content={tooltipText} />}
If following this suggestion: ReportTitleInput.jsx should also be edited
| @@ -0,0 +1,19 @@ | |||
| export const TOOLTIP_CONTENT = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modify object name to camelCase to match project pattern
|
I know you used AI here, i know... |
ArthurCRodrigues
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drumondpucminas needs review
Added new files:
app-grader-builder/src/shared/Tooltip.tsx- Default Tooltip data;app-grader-builder/src/components/feedback/config/TooltipContent.ts- Tooltip content for simplified customization;app-grader-builder/src/shared/index.ts.Changes:
ToggleSwitch.tsxandReportTittleImput.tsx, changing the data input logicfrom string to ReactNodeto support Tooltip functionality. I'm not sure if this might affect system functionality;index.tsx.Note:
It was suggested to use an SVG icon from "hugeicons", but I found it more appropriate to use an Inline SVG to avoid external library dependencies and allow for easier future manipulation.