Conversation
📝 Walkthrough""" WalkthroughThis change extends multiple GraphQL fragments and TypeScript form models to include new language-related fields Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/scenes/Partners/Detail/Tabs/Profile/PartnerTypesSection.tsx (1)
56-56: Reevaluate fixed maxWidth for responsiveness
IncreasingmaxWidthto 700px may cause overflow or cramped layouts on narrow viewports. Consider using responsive breakpoints or fluid sizing (for example:maxWidth={{ xs: '100%', md: 700 }}) to maintain a flexible, mobile-friendly layout.src/scenes/Partners/Detail/PartnerDetail.graphql (1)
68-71: Fix inconsistent property orderThe order of
canEditandcanReadproperties inlanguagesOfConsultingis different from the other language fields. While functionally equivalent, maintaining consistent ordering improves code readability.- canEdit - canRead + canRead + canEditsrc/scenes/Partners/Detail/Tabs/Profile/PartnerLanguageSection.tsx (1)
53-158: Consider refactoring to reduce code duplicationThe code for rendering "Language of Reporting" and "Language of Wider Communication" sections is nearly identical. Consider refactoring this into a reusable function or component to reduce duplication and improve maintainability.
You could create a helper function like:
const renderLanguageSection = ( title: string, languageField: { canRead: boolean; value: { name: { value: string } } | null; }, redactedMessage: string ) => ( <Box> <Typography component="h4" variant="body2" color="textSecondary" gutterBottom> {title} </Typography> {!partner ? ( <Skeleton width="75%" /> ) : languageField.canRead ? ( languageField.value ? ( <Stack component="ul" sx={{ m: 0, p: 0, gap: 1 }}> <Typography component="li" variant="h4" sx={{ listStyleType: 'none' }}> {languageField.value.name.value} </Typography> </Stack> ) : ( <Typography component="p" variant="h4"> None </Typography> ) ) : ( <Redacted info={redactedMessage} width="75%" /> )} </Box> );Then use it in your JSX:
{renderLanguageSection( "Language of Reporting", partner?.languageOfReporting, "You cannot view this partner's reporting language" )} {renderLanguageSection( "Language of Wider Communication", partner?.languageOfWiderCommunication, "You cannot view this partner's language of wider communication" )}src/scenes/Partners/Edit/EditPartner.tsx (1)
119-137: Consider consistent handling of CreateDialogForm across language fieldsThe
CreateDialogFormprop is explicitly set toundefinedfor reporting and wider communication language fields, but not specified for consulting languages field. This inconsistency should be addressed for better maintainability.Either add the prop to the consulting languages field:
- 'partner.languagesOfConsulting': ({ props }) => ( - <LanguageField {...props} multiple label="Languages of Consulting" /> + 'partner.languagesOfConsulting': ({ props }) => ( + <LanguageField + {...props} + multiple + label="Languages of Consulting" + CreateDialogForm={undefined} + />Or document why the behavior is different for this field if that's intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/LanguageListItemCard/LanguageListItem.graphql(1 hunks)src/components/form/Lookup/Language/LanguageLookup.graphql(1 hunks)src/scenes/Languages/Detail/LanguageDetail.graphql(1 hunks)src/scenes/Languages/Detail/LanguageDetail.tsx(1 hunks)src/scenes/Languages/Edit/EditLanguage.tsx(1 hunks)src/scenes/Languages/LanguageForm/LanguageForm.tsx(2 hunks)src/scenes/Languages/LanguageForm/LangugeForm.graphql(1 hunks)src/scenes/Partners/Detail/PartnerDetail.graphql(1 hunks)src/scenes/Partners/Detail/Tabs/Profile/PartnerDetailProfile.tsx(2 hunks)src/scenes/Partners/Detail/Tabs/Profile/PartnerLanguageSection.tsx(1 hunks)src/scenes/Partners/Detail/Tabs/Profile/PartnerTypesSection.tsx(1 hunks)src/scenes/Partners/Edit/EditPartner.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/scenes/Languages/Detail/LanguageDetail.tsx (1)
src/components/BooleanProperty/BooleanProperty.tsx (1)
BooleanProperty(21-40)
src/scenes/Languages/LanguageForm/LanguageForm.tsx (2)
src/common/secured.ts (1)
canReadAny(66-78)src/components/form/SecuredField.tsx (1)
SecuredField(39-74)
🔇 Additional comments (14)
src/components/form/Lookup/Language/LanguageLookup.graphql (1)
19-24: Regenerate GraphQL types after extending LanguageLookupItem
You've addedisWiderComm { value }andisAvailableForReporting { value }to theLanguageLookupItemfragment. Ensure your GraphQL schema supports these fields and run your codegen tooling (e.g.,yarn graphql-codegen) so that the new fields propagate into the generated TypeScript types and downstream components.src/components/LanguageListItemCard/LanguageListItem.graphql (1)
20-25: Update GraphQL codegen for new list item flags
TheLanguageListItemfragment now includesisWiderComm { value }andisAvailableForReporting { value }. Confirm these match the server schema and regenerate your client types so components consuming this fragment (such asPartnerLanguageSection) can safely access the new flags.src/scenes/Languages/Edit/EditLanguage.tsx (1)
58-59: Map new language flags into initial form values
You've addedisAvailableForReporting: language.isAvailableForReporting.valueandisWiderComm: language.isWiderComm.valueto theinitialValuesobject. Verify thatLanguageFormdefines inputs (e.g., secured checkboxes) bound to these fields and that the form’s dirty-state tracking correctly captures changes to them.src/scenes/Languages/LanguageForm/LangugeForm.graphql (1)
91-100: Add new secured fields to LanguageForm fragment; regenerate types
TheLanguageFormfragment now includesisWiderCommandisAvailableForReportingwithcanEdit,canRead, andvaluesubfields. Ensure that the form UI renders these as secured checkbox fields and that you refresh your GraphQL codegen outputs so the new fields are available in your TypeScript definitions.src/scenes/Languages/Detail/LanguageDetail.graphql (1)
80-89: LGTM! The new language capability fields are properly structured.The new
isWiderCommandisAvailableForReportingfields follow the established pattern for secured fields with the necessary permission properties (canEdit,canRead, andvalue).src/scenes/Partners/Detail/Tabs/Profile/PartnerDetailProfile.tsx (2)
8-8: Correct import of the new language section component.The import statement is properly added for the new
PartnerLanguageSectioncomponent.
39-50: Properly implemented language section in the partner profile.The new language section is correctly integrated into the partner profile grid layout. The
onEditcallback properly handles the three language-related fields: reporting language, wider communication language, and consulting languages.src/scenes/Languages/Detail/LanguageDetail.tsx (1)
168-181: Language capability indicators correctly implemented.The new grid with BooleanProperty components for reporting and wider communication language capabilities is well structured. Both components include appropriate labels and redacted messages for permission handling.
src/scenes/Languages/LanguageForm/LanguageForm.tsx (2)
89-95: Modern styling approach using the sx prop.Good modernization by replacing makeStyles with direct styling via the sx prop for the overflow handling.
331-356: Well-structured communication section with proper permission handling.The new Communication section is:
- Correctly conditional based on read permissions using
canReadAny- Properly styled with margin for visual separation
- Well organized with appropriate form fields
- Correctly implements permission-based rendering with
SecuredFieldcomponentsThe checkbox labels clearly communicate the purpose of each field.
src/scenes/Partners/Detail/PartnerDetail.graphql (1)
60-83: New language-related fields added correctlyThe addition of three language-related fields (
languageOfWiderCommunication,languagesOfConsulting, andlanguageOfReporting) to the Partner fragment follows the established pattern in the codebase. Each field correctly includes permission fields and nested fragments.src/scenes/Partners/Edit/EditPartner.tsx (3)
44-46: Language fields properly added to PartnerFormValuesThe language fields are correctly added to the PartnerFormValues type, maintaining type safety for the form implementation.
176-179: Initial values correctly set for language fieldsThe language fields are properly initialized from the partner data. The form correctly handles null values for single language fields and the array for consulting languages.
201-206: Language fields correctly included in mutation variablesThe submission logic correctly processes the language fields, extracting IDs where needed and handling null values appropriately.
src/scenes/Partners/Detail/Tabs/Profile/PartnerLanguageSection.tsx
Outdated
Show resolved
Hide resolved
src/scenes/Partners/Detail/Tabs/Profile/PartnerLanguageSection.tsx
Outdated
Show resolved
Hide resolved
rdonigian
force-pushed
the
partner-location
branch
from
deea7a9 to
acc16cf
Compare
CarsonF
left a comment
CarsonF
left a comment
There was a problem hiding this comment.
I rebased this and dropped most of the isWiderComm flag. I left some comments here, and will do a final pass tomorrow.
| WithValueGetterReturning< | ||
| Pick<LanguageLookupItem, 'displayName' | 'name' | 'id'>, | ||
| Row | ||
| > |
There was a problem hiding this comment.
I don't like picking apart fragments. IMO they should be used 1-1 between GQL-TS.
If columns need a different language shape, then they should declare their own fragment to be used.
Strictly, this is not necessary for this case either, I would just revert.
| isWiderComm { | ||
| value | ||
| } | ||
| isAvailableForReporting { | ||
| value | ||
| } |
There was a problem hiding this comment.
Remove these. This prop is not consumed in the <LanguageListItem> component.
| } | ||
| } | ||
| isAvailableForReporting { | ||
| canEdit |
There was a problem hiding this comment.
not used in language detail
| canEdit |
| ...LanguageListItem | ||
| ...LanguageLookupItem |
There was a problem hiding this comment.
<LanguageListItem> is not used here, so don't use its fragment.
Declare the fields read inline, or creating a new fragment is fine too.
LanguageLookupItem is fine here, since it's used in the form. But we shouldn't rely on what fields it asks for either for my above statement.
| <Stack component="ul" sx={{ m: 0, p: 0, gap: 1 }}> | ||
| <Typography | ||
| component="li" | ||
| variant="h4" | ||
| sx={{ listStyleType: 'none' }} | ||
| > | ||
| {partner.languageOfReporting.value.name.value} | ||
| </Typography> | ||
| </Stack> |
There was a problem hiding this comment.
This data point is not a list, so it shouldn't use <ul>/<li>
| countries: readonly DisplayLocationFragment[]; | ||
| languageOfReportingId: LanguageLookupItem | null; | ||
| languageOfWiderCommunicationId: LanguageLookupItem | null; | ||
| languagesOfConsulting: PartnerDetailsFragment['languagesOfConsulting']['value']; |
There was a problem hiding this comment.
| languagesOfConsulting: PartnerDetailsFragment['languagesOfConsulting']['value']; | |
| languagesOfConsulting: readonly LanguageLookupItem[]; |
| {...props} | ||
| label="Language of Wider Communication" | ||
| CreateDialogForm={undefined} | ||
| getOptionDisabled={(option) => !option.isWiderComm.value} |
There was a problem hiding this comment.
| getOptionDisabled={(option) => !option.isWiderComm.value} |
| /> | ||
| ), | ||
| 'partner.languagesOfConsulting': ({ props }) => ( | ||
| <LanguageField {...props} multiple label="Languages of Consulting" /> |
There was a problem hiding this comment.
outlined variant looks better for multiple values aka chips. the gray chips on filled variant gray bg looks bad.
| <LanguageField {...props} multiple label="Languages of Consulting" /> | |
| <LanguageField {...props} multiple label="Languages of Consulting" variant="outlined" /> |
| <LanguageField | ||
| {...props} | ||
| label="Language of Reporting" | ||
| CreateDialogForm={undefined} |
There was a problem hiding this comment.
I would not try to forcibly disable language creation here. This is a deviation from the standard practice of the app. It is also not necessary because only admins can create languages, so only admins would have the create language option anyways.
2 participants
API PR: SeedCompany/cord-api-v3#3438
Partner language section will expose reporting language, wider communication language, and consulting language details to the UI.