Conversation
…ypes and update documentation for optional properties
…on for computeRowIndices method
…structure and maintainability
…feat/SFT-2638-ai-research # Conflicts: # packages/plugin/src/Resources/js/client/client.js # packages/plugin/src/Resources/js/client/vendor.js
… into feat/SFT-2638-ai-research # Conflicts: # packages/plugin/src/Resources/js/client/client.js
…ce/craft-freeform into feat/SFT-2638-ai-research
…oad fields in Table fields (#2448) Co-authored-by: kjmartens <kelsey@solspace.com>
… into feat/SFT-2638-ai-research # Conflicts: # packages/plugin/src/Resources/js/client/client.js # packages/plugin/src/Resources/js/client/vendor.js
… into feat/SFT-2638-ai-research
…ce/craft-freeform into feat/SFT-2638-ai-research
… into feat/SFT-2638-ai-research
…ce/craft-freeform into feat/SFT-2638-ai-research
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: Gustavs Gūtmanis <gustavs@solspace.com>
… into feat/SFT-2638-ai-research # Conflicts: # packages/client/src/index.tsx # packages/plugin/src/Resources/js/client/client.js # packages/plugin/src/Resources/js/client/vendor.js # packages/plugin/src/Resources/js/client/vendor.js.LICENSE.txt # packages/plugin/src/Resources/js/scripts/front-end/plugin/freeform.js
…feat/v5.15 # Conflicts: # composer.json # packages/plugin/src/Resources/js/client/client.js
… into feat/SFT-2638-ai-research # Conflicts: # packages/plugin/src/Resources/js/client/client.js # packages/plugin/src/Resources/js/client/vendor.js
| payment_history?: { | ||
| package_price?: number; | ||
| credits?: number; | ||
| amount_added?: number; | ||
| paid_at?: string; | ||
| }[]; |
There was a problem hiding this comment.
Nested types which are used as Arrays should be separated out, and their type used.
type PaymentHistory = {
package_price?: number;
credits?: number;
amount_added?: number;
paid_at?: string;
}
type AiUsageResponse {
// ...
payment_history: PaymentHistory[];
}| return ( | ||
| <div> | ||
| <Breadcrumb id="ai" label="AI" url="ai" /> | ||
| <HeaderContainer>{translate('AI')}</HeaderContainer> | ||
| <DashboardWrapper> | ||
| <CardsGrid> | ||
| <Card> | ||
| <CardLabel> | ||
| <Skeleton width={80} height={10} /> | ||
| </CardLabel> |
There was a problem hiding this comment.
This is so big that it should be in its own file.
I would also move the forbidden and not found in their own files. Actually, just one file, since they're identical, except the messaging. A single boolean prop would determine which copy to display.
| function formatDate(iso: string | null | undefined): string { | ||
| if (!iso) return '—'; | ||
| try { | ||
| const d = new Date(iso); | ||
| return Number.isNaN(d.getTime()) | ||
| ? iso | ||
| : d.toLocaleDateString(undefined, { dateStyle: 'medium' }); | ||
| } catch { | ||
| return iso; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this function inline here?
It gets recreated on every re-render.
There's nothing that uses anything in the scope, it should be moved to a utility file and imported and used that way.
Also we have date-fns library, which should be used for formatting dates.
And please refrain from naming variables as a, b, c, etc.
| if (isError) { | ||
| const message = | ||
| axios.isAxiosError(error) && error.response?.data?.message | ||
| ? error.response.data.message | ||
| : error instanceof Error | ||
| ? error.message | ||
| : translate('Failed to load usage data'); | ||
| return ( | ||
| <div> | ||
| <Breadcrumb id="ai" label="AI" url="ai" /> | ||
| <HeaderContainer>{translate('AI')}</HeaderContainer> | ||
| <DashboardWrapper> | ||
| <EmptyState> | ||
| <EmptyStateTitle> | ||
| {translate('Error loading usage')} | ||
| </EmptyStateTitle> | ||
| <p>{message}</p> | ||
| </EmptyState> | ||
| </DashboardWrapper> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
too many inline cases of rendering something completely different.
All of them share the same layout, so at least bring the layout wrappers out to a separate component and use that by passing only the children, that would make this file have less filler code which makes it difficult to navigate and understand.
| {summary.mode === 'trial' && translate('Free trial')} | ||
| {summary.mode === 'plan' && translate('Active plan')} | ||
| {summary.mode === 'blocked' && | ||
| translate('Usage limit reached')} | ||
| {(!summary.mode || summary.mode === 'unknown') && | ||
| translate('Not configured')} |
There was a problem hiding this comment.
I would suggest writing a little function which does this, and then pass the mode to it.
const getSummaryModeLabel = (mode: SummaryMode): string => {
switch (mode) {
case "trial":
return "Free Trial";
// ...
}
}
// ...
<CardValueSmall>
{getSummaryModeLabel(summary.mode)}
</CardValueSmall>| <MetricsTableRow key={b.key}> | ||
| <MetricsTableCell> | ||
| {formatBundlePrice( | ||
| b.price, |
There was a problem hiding this comment.
b. Please use bundle, or do bundles.map(({ price, currency } or something
| const promptProperty = useMemo<TextareaProperty>( | ||
| () => ({ | ||
| type: PropertyType.Textarea, | ||
| handle: 'prompt', | ||
| label: translate('Describe your form'), | ||
| instructions: translate('Describe the fields and purpose of the form.'), | ||
| required: true, | ||
| rows: 4, | ||
| placeholder: translate( | ||
| 'e.g. Contact form with name, email, phone, and a message box' | ||
| ), | ||
| }), | ||
| [] | ||
| ); |
There was a problem hiding this comment.
these can be simple objects.
also - since the v5.15 changes you can pass these props directly to the <Control> without needing a property variable, so that makes it easier to use control components
| let message: string | null = null; | ||
| if (err && typeof err === 'object' && 'response' in err) { | ||
| const res = (err as { response?: { data?: unknown } }).response?.data; | ||
| if (typeof res === 'string') { | ||
| message = res; | ||
| } else if ( | ||
| res && | ||
| typeof res === 'object' && | ||
| 'message' in res && | ||
| typeof (res as { message: unknown }).message === 'string' | ||
| ) { | ||
| message = (res as { message: string }).message; | ||
| } | ||
| } | ||
| setError( | ||
| message || | ||
| translate('Form generation failed. Please try again or rephrase.') | ||
| ); |
There was a problem hiding this comment.
this looks very GPT-y 😆
it would be very nice to write a handler function for extracting error messages from AJAX errors, or just use the optimistic path and assume we know how the error looks.
| <button className="btn submit add icon" onClick={openCreateFormModal}> | ||
| {translate('Create a new Form')} | ||
| </button> | ||
| <div style={{ display: 'flex', gap: '0.5rem', flexWrap: 'wrap' }}> |
There was a problem hiding this comment.
Please write a new styled component for this. Do not use inline styles
| 'plan' => $body['plan'] ?? null, | ||
| 'bundle_key' => $body['bundle_key'] ?? null, | ||
| 'currency' => $body['currency'] ?? null, | ||
| ], static function ($v): bool { return $v !== null && $v !== ''; }); |
There was a problem hiding this comment.
This looks like a prime candidate for shorthand anonymous functions
| ], static function ($v): bool { return $v !== null && $v !== ''; }); | |
| ], static fn ($v) => $v !== null && $v !== ''); |
…feat/v5.15 # Conflicts: # packages/plugin/src/Resources/js/scripts/front-end/plugin/freeform.js
… into feat/SFT-2638-ai-research
…ed readability and functionality
3 participants
No description provided.