Tech Debt - Removing makeStyles #1746
Conversation
📝 WalkthroughWalkthroughThis pull request systematically migrates styling across 50+ components from Material-UI's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PartnerListItemCard/PartnerListItemCard.tsx (1)
11-11: Remove theclassNameprop for consistency and code cleanliness.The
classNameprop is unused throughout the codebase—none of the 4 usages ofPartnerListItemCardpass this prop. Additionally, similar components likeProjectListItemCardandLanguageListItemCarddon't exposeclassName, and the modern pattern in this codebase favorsStylePropsover rawclassNameprops. Remove it from the interface and component signature.
🧹 Nitpick comments (22)
src/components/ProgressButton/ProgressButton.tsx (1)
34-52: Consider migrating toBoxcomponent withsxprop for consistency with the PR migration goals.This component currently uses inline
styleprops on raw DOM elements rather than Material-UI'ssxprop. Given the PR's explicit goal of migrating tosx-based styling, refactoring the wrapperdivandspanto useBoxcomponents would better align with the broader tech debt effort and provide access to theme tokens.This is optional and can be deferred, but would strengthen consistency across the codebase.
Apply this diff as an optional enhancement:
-import { +import { + Box, Button, ButtonProps, CircularProgress, CircularProgressProps, -} from '@mui/material'; +} from '@mui/material'; const inner = ( <> {progress ? ( - <div - // This is to center spinner within button, while maintaining consistent button width. + <Box + // This is to center spinner within button, while maintaining consistent button width. // If we were to replace the button text, the button size could change which we want to // avoid because the buttons shifting around is jarring for the user. Esp since the // spinner is only shown for a short time. - style={{ + sx={{ position: 'absolute', inset: 0, display: 'flex', alignItems: 'center', justifyContent: 'center', }} > <CircularProgress size={size === 'large' ? 26 : size === 'small' ? 16 : 20} color={!rest.disabled ? 'inherit' : 'primary'} {...progressProps} /> - </div> + </Box> ) : null} - <span style={{ visibility: progress ? 'hidden' : undefined }}> + <Box component="span" sx={{ visibility: progress ? 'hidden' : undefined }}> {children} - </span> + </Box> </> );Also applies to: 54-56
src/components/Upload/UploadItems.tsx (1)
17-27: Consider removing commented debug code.The commented-out test data and component usage appear to be development artifacts that can be safely removed for cleaner code.
Also applies to: 55-59
src/components/posts/PostListItemCard/PostListItemMenu.tsx (1)
28-28: Consider extracting the repeatedsxobject.The same
sxobject{ mr: 2, minWidth: 'unset' }is duplicated on both ListItemIcon components. Extracting it to a constant would reduce duplication and improve maintainability.Apply this diff to extract the common styling:
export const PostListItemMenu = (props: PostListItemMenuProps) => { const { onEdit, onDelete, ...rest } = props; + const iconSx = { mr: 2, minWidth: 'unset' }; return ( <Menu id="post-options-menu" keepMounted open={Boolean(props.anchorEl)} anchorOrigin={{ vertical: 'top', horizontal: 'right' }} transformOrigin={{ vertical: 'top', horizontal: 'right' }} {...rest} > <MenuItem onClick={onEdit}> - <ListItemIcon sx={{ mr: 2, minWidth: 'unset' }}> + <ListItemIcon sx={iconSx}> <EditIcon fontSize="small" /> </ListItemIcon> <ListItemText primary="Edit Post" /> </MenuItem> <MenuItem onClick={onDelete}> - <ListItemIcon sx={{ mr: 2, minWidth: 'unset' }}> + <ListItemIcon sx={iconSx}> <DeleteIcon fontSize="small" /> </ListItemIcon> <ListItemText primary="Delete Post" /> </MenuItem> </Menu> ); };Also applies to: 34-34
src/components/form/VersesField.tsx (1)
194-204: Consider using theme values for colors and typography.The inline styles work correctly, and the migration from
makeStylestosxis implemented properly. However, using hard-coded hex colors and pixel values reduces maintainability and prevents these styles from adapting to theme changes or dark mode.Consider refactoring to use theme values:
sx={{ - color: '#FFFFFF', - backgroundColor: '#2D9CDB', + color: 'common.white', + backgroundColor: 'primary.main', fontWeight: 600, - lineHeight: '32px', + lineHeight: 2, margin: 2.5, - fontSize: 16, + fontSize: 'body1.fontSize', '& .MuiChip-deleteIcon': { - color: '#FFFFFF', + color: 'inherit', }, }}This approach uses Material-UI's theme system, making the component more flexible and consistent with the rest of the application. Adjust the specific theme paths (
primary.main,body1.fontSize) to match your design system's values.src/components/FundingAccountCard/FundingAccountCard.tsx (1)
11-11: Consider tracking the removal candidate in an issue.The inline comment about this being a candidate for removal would be better tracked as a GitHub issue or TODO with context. If removal is not planned, consider removing this comment.
src/scenes/Partnerships/List/PartnershipList.tsx (1)
47-53: Consider using consistent shorthand notation.The header Box uses explicit
theme.spacing(3, 0)while the Typography uses shorthandmr: 3. For consistency, you could simplify the header Box styling to use shorthand notation as well.Apply this diff for consistency:
<Box - sx={(theme) => ({ - margin: theme.spacing(3, 0), + sx={{ + my: 3, display: 'flex', - })} + }} >src/components/InternshipEngagementListItemCard/InternshipEngagementListItemCard.tsx (1)
20-21: Consider removing the redundant type alias.The
InternshipEngagementListItemCardPropstype is now just an alias toInternshipEngagementListItemFragmentwith no additional properties. Moreover, the component itself (line 24) uses the fragment type directly rather than this exported type, creating an inconsistency.Consider one of these options:
Option 1 (recommended): Remove the type alias entirely since it adds no value:
-export type InternshipEngagementListItemCardProps = - InternshipEngagementListItemFragment; - export const InternshipEngagementListItemCard = ( props: InternshipEngagementListItemFragment ) => {Option 2: Keep the type for API stability but use it consistently:
export type InternshipEngagementListItemCardProps = InternshipEngagementListItemFragment; export const InternshipEngagementListItemCard = ( - props: InternshipEngagementListItemFragment + props: InternshipEngagementListItemCardProps ) => {src/components/Upload/UploadItem.tsx (2)
44-59: Consider removing redundant ellipsis styles from Stack.The ellipsis-handling styles (overflow, textOverflow, whiteSpace) are applied to both the Stack (lines 47-49) and the Typography (lines 55-57). Since the Typography alone handles text truncation, the Stack only needs the overflow: 'hidden' to contain the Typography.
Apply this diff to simplify:
<Stack sx={(theme) => ({ mr: theme.spacing(2), overflow: 'hidden', - textOverflow: 'ellipsis', - whiteSpace: 'nowrap', })} >
97-100: Use theme.spacing() for consistency instead of fixed pixels.The Box padding uses a fixed pixel value ('12px') instead of theme-based spacing, which is inconsistent with the approach used elsewhere in this component (lines 37, 46).
Apply this diff:
<Box - sx={{ - p: '12px', - }} + sx={(theme) => ({ + p: theme.spacing(1.5), + })} >src/components/UserListItemCard/LandscapeCard.tsx (1)
44-44: Consider using Box component for consistency.This div uses inline
styleprop while the rest of the migration uses Material-UI'ssxprop. For consistency with the styling migration pattern, consider using a Box component instead.- <div style={{ flex: 1 }}> + <Box sx={{ flex: 1 }}> <Typography variant="h4" paragraph> {!user ? ( <Skeleton width="75%" /> @@ -55,7 +55,7 @@ <Typography variant="body2" color="textSecondary"> {!user ? <Skeleton width="50%" /> : user.title.value} </Typography> - </div> + </Box>And add Box to the imports:
import { Box, Card, CardContent, Skeleton, Typography } from '@mui/material';src/components/form/EnumField.tsx (1)
293-296: Migration looks good; optionally verify theme path.The bold font weight styling is correctly migrated to the
sxprop. The theme paththeme.typography.weight.boldappears to be a custom extension. If not already tested, you may want to verify this path exists in your theme configuration.If you'd like, I can verify the theme configuration:
#!/bin/bash # Search for theme definitions that include typography.weight rg -n -A5 -B5 'typography.*weight|weight.*bold' --type=ts --type=tsx -g '!node_modules' -g '!dist' -g '!build'src/components/files/FileVersionItem/FileVersionItem.tsx (1)
51-58: LGTM! Successful migration to sx-based styling.The component correctly migrates from makeStyles to inline sx props. All spacing and styling values are properly specified.
Optional: For consistency, you could use either all shorthand spacing (e.g.,
marginRight: 2) or all explicit theme.spacing() calls (e.g.,marginRight: theme.spacing(2),minWidth: theme.spacing(4)), but both approaches work correctly as-is.src/components/ProjectChangeRequestListItem/ProjectChangeRequestListItem.tsx (2)
49-49: LGTM! Consider Typography for semantic consistency (optional).The Box separator works correctly with theme-aware styling. For minor semantic improvement, you could use
Typography component="span"instead of Box, since it's displaying text content.Optional refactor:
- <Box sx={{ color: 'text.secondary', mx: 1 }}>—</Box> + <Typography component="span" sx={{ color: 'text.secondary', mx: 1 }}>—</Typography>
97-97: LGTM! Spacer implementation is functional.The inline style approach works correctly for this simple flex spacer. For complete consistency with the sx-based migration, you could optionally use
<Box sx={{ flexGrow: 1 }} />, but the current implementation is perfectly acceptable.src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
23-23: Consider usingsxprop instead ofstylefor consistency.The migration to sx-based styling is used for the
SensitivityIcon(line 36-40), but theBreadcrumbcontainer still uses the inlinestyleprop. For consistency with the rest of the PR's migration pattern, consider usingsxinstead ofstyle.Apply this diff:
- style={{ display: 'flex', alignItems: 'center', ...rest.style }} + sx={{ display: 'flex', alignItems: 'center', ...rest.sx }}Note: This would also require updating the interface if
BreadcrumbPropsexpectsstylerather thansx.src/scenes/Engagement/InternshipEngagement/MentorCard/MentorCard.tsx (1)
22-28: Consider using a MUIBoxcomponent withsxinstead of adivwith inlinestyle.For consistency with the rest of this PR's migration pattern (which uses
sxprops throughout), consider replacing the div with inline style with a MUI Box component using thesxprop.Apply this diff:
+import { Box } from '@mui/material'; ... - <div - style={{ - height: '100%', - display: 'flex', - flexDirection: 'column', - }} - > + <Box + sx={{ + height: '100%', + display: 'flex', + flexDirection: 'column', + }} + > <Typography ... - </div> + </Box>src/scenes/Partners/Detail/Tabs/People/PartnerDetailPeople.tsx (1)
32-52: LGTM! Consider extracting common pattern.The
sxprop usage on CardActionArea and Avatar is correct and follows MUI best practices. The styling is nearly identical to the MentorCard component (lines 42-60), which suggests a potential opportunity for extracting a shared "AddPersonCard" or similar component to reduce duplication.src/components/MemberListSummary/MemberListSummary.tsx (1)
58-58: Consider using a MUIBoxcomponent withsxinstead of adivwith inlinestyle.For consistency with the rest of this PR's migration pattern (which uses
sxprops throughout), consider replacing the div with inline style with a MUI Box component using thesxprop.Apply this diff:
+import { Box } from '@mui/material'; ... - <div style={{ display: 'flex', alignItems: 'center' }}> + <Box sx={{ display: 'flex', alignItems: 'center' }}> <AvatarGroup max={max} sx={{ mr: 1 }}> ... - </div> + </Box>src/components/posts/PostListItemCard/PostListItemCard.tsx (1)
69-69: Replace inline style with Box and sx for consistency.Line 69 uses an inline
styleprop, which is inconsistent with the sx-based styling used throughout the rest of this component and the broader migration.Apply this diff to align with the sx-based approach:
- <div style={{ flex: 1 }}> + <Box sx={{ flex: 1 }}>And update the closing tag at line 120:
</div> + </Box> </CardContent>src/components/ProjectMemberCard/ProjectMemberCard.tsx (1)
46-46: Replace inline style with Box and sx for consistency.Line 46 uses an inline
styleprop, which is inconsistent with the sx-based styling used throughout the rest of this component and the broader migration.Apply this diff to align with the sx-based approach:
- <div style={{ flexGrow: 1 }}> + <Box sx={{ flexGrow: 1 }}>And update the closing tag at line 62:
- </div> + </Box> </CardContent>Note: You'll need to ensure
Boxis imported from@mui/material.src/components/LanguageListItemCard/LanguageListItemCard.tsx (1)
57-68: Consider using sx instead of inline styles for consistency.Lines 57-68 use the
styleprop with inline styles, while the rest of this file and the broader migration usesxprops. For consistency with the rest of the refactoring, consider converting these tosx.Apply this diff to align with the sx-based approach:
- <div - style={{ - display: 'flex', - justifyContent: 'space-between', - alignItems: 'flex-end', - }} - > - <div - style={{ - flex: 1, - }} - > + <Box + sx={{ + display: 'flex', + justifyContent: 'space-between', + alignItems: 'flex-end', + }} + > + <Box + sx={{ + flex: 1, + }} + > <DisplaySimpleProperty LabelProps={{ color: 'textSecondary' }} label="Ethnologue Code" value={language?.ethnologue.code.value} loading={!language} loadingWidth="25%" /> <DisplaySimpleProperty LabelProps={{ color: 'textSecondary' }} label="Registry of Language Varieties Code" value={language?.registryOfLanguageVarietiesCode.value} loading={!language} loadingWidth="25%" /> <Sensitivity value={language?.sensitivity} loading={!language} sx={{ mt: 1, }} /> - </div> + </Box>src/components/OrganizationListItemCard/OrganizationListItemCard.tsx (1)
19-25: Consider the implications of mixingclassNameandsxprops.The Card receives both
className(line 20) andsx(lines 21-24) props. While MUI supports this pattern,sxhas higher specificity and will overrideclassNamefor the same properties. This could lead to unexpected behavior if consumers pass aclassNameexpecting it to control width or maxWidth.If
classNamesupport is intentional for backwards compatibility, consider documenting this in a comment or the component's documentation to clarify the precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
src/components/Avatar/Avatar.stories.tsx(2 hunks)src/components/Avatar/Avatar.tsx(1 hunks)src/components/BudgetOverviewCard/BudgetOverviewCard.tsx(0 hunks)src/components/Changeset/ChangesetBanner.tsx(1 hunks)src/components/Changeset/PropertyDiff.tsx(1 hunks)src/components/Dialog/DialogForm.tsx(1 hunks)src/components/Error/Error.tsx(3 hunks)src/components/FieldOverviewCard/FieldOverviewCard.tsx(3 hunks)src/components/FundingAccountCard/FundingAccountCard.tsx(1 hunks)src/components/Icons/HugeIcon.stories.tsx(1 hunks)src/components/Icons/HugeIcon.tsx(1 hunks)src/components/InternshipEngagementListItemCard/InternshipEngagementListItemCard.tsx(1 hunks)src/components/LanguageEngagementListItemCard/LanguageEngagementListItemCard.tsx(4 hunks)src/components/LanguageListItemCard/LanguageListItemCard.tsx(6 hunks)src/components/List/List.tsx(4 hunks)src/components/LocationCard/LocationCard.tsx(4 hunks)src/components/MemberListSummary/MemberListSummary.tsx(2 hunks)src/components/MethodologiesCard/MethodologiesCard.tsx(3 hunks)src/components/OrganizationListItemCard/OrganizationListItemCard.tsx(1 hunks)src/components/PartnerListItemCard/PartnerListItemCard.tsx(2 hunks)src/components/PartnershipCard/PartnershipCard.tsx(3 hunks)src/components/PartnershipCard/PartnershipPrimaryIcon.tsx(1 hunks)src/components/ProgressButton/ProgressButton.tsx(1 hunks)src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx(2 hunks)src/components/ProjectChangeRequestListItem/ProjectChangeRequestListItem.tsx(5 hunks)src/components/ProjectListItemCard/ProjectListItemCard.tsx(8 hunks)src/components/ProjectMemberCard/ProjectMemberCard.tsx(2 hunks)src/components/Redacted/Redacted.tsx(2 hunks)src/components/Sensitivity/Sensitivity.tsx(1 hunks)src/components/Sort/SortOption.tsx(1 hunks)src/components/Upload/UploadItem.tsx(2 hunks)src/components/Upload/UploadItems.tsx(1 hunks)src/components/UserListItemCard/LandscapeCard.tsx(2 hunks)src/components/UserListItemCard/PortraitCard.tsx(1 hunks)src/components/files/FileActions/FileActionsMenu.tsx(2 hunks)src/components/files/FileActions/FileVersions.tsx(2 hunks)src/components/files/FileVersionItem/FileVersionItem.tsx(1 hunks)src/components/form/EnumField.tsx(3 hunks)src/components/form/NumberField.tsx(1 hunks)src/components/form/VersesField.tsx(1 hunks)src/components/posts/PostList.tsx(1 hunks)src/components/posts/PostListItemCard/PostListItemCard.tsx(2 hunks)src/components/posts/PostListItemCard/PostListItemMenu.tsx(1 hunks)src/scenes/Engagement/InternshipEngagement/MentorCard/MentorCard.tsx(2 hunks)src/scenes/Languages/Detail/LanguageDetail.tsx(9 hunks)src/scenes/Partners/Detail/Tabs/People/PartnerDetailPeople.tsx(1 hunks)src/scenes/Partnerships/List/PartnershipList.tsx(4 hunks)src/scenes/Products/List/ProductList.tsx(1 hunks)src/scenes/Projects/ChangeRequest/List/ProjectChangeRequestList.tsx(4 hunks)src/scenes/Projects/Members/List/ProjectMembersList.tsx(4 hunks)src/scenes/Projects/Overview/ProjectOverview.tsx(7 hunks)
💤 Files with no reviewable changes (1)
- src/components/BudgetOverviewCard/BudgetOverviewCard.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T17:50:15.534Z
Learnt from: CarsonF
Repo: SeedCompany/cord-field PR: 1696
File: src/scenes/FieldZones/CreateFieldZone/CreateFieldZone.tsx:0-0
Timestamp: 2025-06-20T17:50:15.534Z
Learning: In this codebase, GraphQL documents (like CreateFieldZoneDocument, CreateFieldRegionDocument, etc.) are already properly typed, and Apollo hooks (useMutation, useQuery, etc.) automatically infer the correct types from these typed documents. Therefore, explicit generic type parameters on Apollo hooks are unnecessary and should not be suggested.
Applied to files:
src/components/files/FileActions/FileVersions.tsxsrc/scenes/Languages/Detail/LanguageDetail.tsx
📚 Learning: 2024-10-21T15:17:35.618Z
Learnt from: CarsonF
Repo: SeedCompany/cord-field PR: 1604
File: src/components/Feature.tsx:12-19
Timestamp: 2024-10-21T15:17:35.618Z
Learning: In `src/components/Feature.tsx`, the `Feature` component uses `useFeatureFlagPayload` and the `match` prop to determine feature availability, which differs from the `useFeatureEnabled` hook.
Applied to files:
src/components/form/EnumField.tsx
🧬 Code graph analysis (29)
src/components/PartnerListItemCard/PartnerListItemCard.tsx (1)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)
src/components/Error/Error.tsx (1)
src/components/Routing/Navigate.tsx (2)
StatusCode(59-65)statusCode(89-91)
src/components/Icons/HugeIcon.tsx (1)
src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)
src/components/FieldOverviewCard/FieldOverviewCard.tsx (1)
src/components/Icons/HugeIcon.tsx (1)
HugeIcon(10-38)
src/components/PartnershipCard/PartnershipCard.tsx (1)
src/components/PartnershipCard/PartnershipPrimaryIcon.tsx (1)
PartnershipPrimaryIcon(5-19)
src/components/OrganizationListItemCard/OrganizationListItemCard.tsx (1)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)
src/scenes/Partnerships/List/PartnershipList.tsx (2)
src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
ProjectBreadcrumb(13-48)src/components/Breadcrumb/Breadcrumb.tsx (1)
Breadcrumb(15-38)
src/components/UserListItemCard/LandscapeCard.tsx (3)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)src/common/styles.ts (1)
square(8-11)
src/components/InternshipEngagementListItemCard/InternshipEngagementListItemCard.tsx (2)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)src/components/Changeset/useChangesetAwareIdFromUrl.ts (1)
idForUrl(28-29)
src/components/LanguageEngagementListItemCard/LanguageEngagementListItemCard.tsx (2)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)src/components/Changeset/useChangesetAwareIdFromUrl.ts (1)
idForUrl(28-29)
src/scenes/Projects/ChangeRequest/List/ProjectChangeRequestList.tsx (2)
src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
ProjectBreadcrumb(13-48)src/components/Breadcrumb/Breadcrumb.tsx (1)
Breadcrumb(15-38)
src/components/Changeset/PropertyDiff.tsx (1)
src/common/sx.ts (2)
Sx(3-3)extendSx(19-22)
src/components/PartnershipCard/PartnershipPrimaryIcon.tsx (1)
src/common/sx.ts (1)
extendSx(19-22)
src/components/Redacted/Redacted.tsx (1)
src/common/sx.ts (1)
extendSx(19-22)
src/scenes/Projects/Members/List/ProjectMembersList.tsx (2)
src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
ProjectBreadcrumb(13-48)src/components/Breadcrumb/Breadcrumb.tsx (1)
Breadcrumb(15-38)
src/components/ProjectMemberCard/ProjectMemberCard.tsx (1)
src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)
src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
src/components/Sensitivity/SensitivityIcon.tsx (1)
SensitivityIcon(13-26)
src/scenes/Partners/Detail/Tabs/People/PartnerDetailPeople.tsx (2)
src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)src/common/styles.ts (1)
square(8-11)
src/components/List/List.tsx (6)
src/api/schema/util.ts (1)
Entity(8-10)src/components/List/useListQuery.ts (1)
ListQueryResult(13-25)src/api/caching/lists/types.ts (1)
PaginatedListOutput(17-22)src/common/sx.ts (2)
StyleProps(5-8)extendSx(19-22)src/hooks/usePersistedScroll.ts (1)
usePersistedScroll(12-45)src/components/Changeset/ChangesetDiffContext.tsx (1)
useDetermineChangesetDiffItem(126-127)
src/scenes/Engagement/InternshipEngagement/MentorCard/MentorCard.tsx (2)
src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)src/common/styles.ts (1)
square(8-11)
src/components/Sensitivity/Sensitivity.tsx (1)
src/common/sx.ts (2)
StyleProps(5-8)extendSx(19-22)
src/components/LanguageListItemCard/LanguageListItemCard.tsx (1)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)
src/scenes/Projects/Overview/ProjectOverview.tsx (1)
src/components/List/List.tsx (1)
List(33-109)
src/components/ProjectListItemCard/ProjectListItemCard.tsx (4)
src/common/sx.ts (2)
StyleProps(5-8)extendSx(19-22)src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)src/scenes/Projects/useProjectId.ts (1)
getProjectUrl(16-17)src/components/DisplaySimpleProperty/DisplaySimpleProperty.tsx (1)
DisplaySimpleProperty(14-50)
src/components/posts/PostListItemCard/PostListItemCard.tsx (4)
src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)src/common/styles.ts (1)
square(8-11)src/components/Formatters/FormattedDate.tsx (1)
FormattedDateTime(68-81)src/components/IconButton/IconButton.tsx (1)
IconButton(12-29)
src/components/LocationCard/LocationCard.tsx (1)
src/common/sx.ts (2)
StyleProps(5-8)extendSx(19-22)
src/components/FundingAccountCard/FundingAccountCard.tsx (1)
src/components/DisplaySimpleProperty/DisplaySimpleProperty.tsx (1)
DisplaySimpleProperty(14-50)
src/components/UserListItemCard/PortraitCard.tsx (3)
src/components/Routing/CardActionAreaLink.tsx (1)
CardActionAreaLink(20-34)src/components/Avatar/Avatar.tsx (1)
Avatar(11-31)src/common/styles.ts (1)
square(8-11)
src/components/Icons/HugeIcon.stories.tsx (1)
src/components/Icons/CordIcon.tsx (1)
CordIcon(4-15)
🪛 Biome (2.1.2)
src/components/Avatar/Avatar.stories.tsx
[error] 25-25: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (73)
src/components/ProgressButton/ProgressButton.tsx (1)
35-38: Clear and helpful comments documenting the layout strategy.The comments effectively explain why absolute positioning and hidden text are necessary to maintain consistent button dimensions during the loading state. This is a valuable addition for future maintainers.
src/components/Upload/UploadItems.tsx (1)
46-48: LGTM! Clean migration to sx prop.The inline
sxprop correctly applies the disabled color from the theme palette. The function form properly accesses the theme object.src/components/Redacted/Redacted.tsx (1)
3-3: LGTM! Clean migration to sx-based styling.The refactor correctly migrates from the old styling approach to the new
sx-based pattern. TheextendSxutility properly normalizes the user-providedSkeletonProps?.sx(whether undefined, object, or array) into an array format, and the base style{ transform: 'initial' }is correctly positioned first, allowing user styles to override it when needed.Also applies to: 25-30
src/components/Sort/SortOption.tsx (1)
44-51: LGTM! Clean migration to sx prop.The styling migration from
makeStylesto inlinesxprop is correctly implemented. The use of theme callback for accessing spacing and typography values maintains consistency with MUI's theming system.src/components/Dialog/DialogForm.tsx (1)
169-169: LGTM! Clean migration from makeStyles to inline style.The spacer implementation using
flex: 1is correct and appropriate for this simple use case. The inline style is lightweight and achieves the PR's goal of removing makeStyles dependencies without adding unnecessary complexity.src/components/FundingAccountCard/FundingAccountCard.tsx (1)
17-32: LGTM! Clean migration to sx props.The migration from
makeStylesto inlinesxprops is well-executed. The component correctly preserves theclassNameprop for backward compatibility while adopting the modern MUI v5 styling approach. The inline styles are appropriate for these straightforward styling needs.src/scenes/Partnerships/List/PartnershipList.tsx (3)
2-2: LGTM!The Box import is correctly added to support the new Box-based layout below.
32-39: LGTM!The root container has been correctly migrated to Box with sx props. The theme callback is appropriately used to access breakpoint values.
67-84: LGTM!The closing tags and overall structure correctly maintain the original layout while using the new Box-based approach.
src/components/InternshipEngagementListItemCard/InternshipEngagementListItemCard.tsx (1)
33-57: LGTM! Styling migration completed correctly.The migration from
makeStylesto inlinesxprops is implemented correctly:
- Simple
widthstyling on Card- Flex layout on CardActionAreaLink
- Theme-aware spacing and flex layout on Grid using the theme function
All styling syntax is valid and appropriate for the component's layout needs.
src/components/Upload/UploadItem.tsx (3)
5-11: LGTM!The import additions are appropriate for the migration from makeStyles to sx-based styling with MUI components.
32-43: LGTM!The container Box styling correctly implements flexbox layout with theme-based spacing and dividers.
89-95: Verify the use ofcolor="error"on the completed state IconButton.The IconButton for the completed state uses
color="error"(line 91), which seems semantically incorrect. Typically, a completed state would use "success" or a neutral color, not "error".If this is intentional for styling purposes, please confirm. Otherwise, consider:
<IconButton aria-label="completed" - color="error" + color="default" onClick={() => console.log('TODO: Add onCompleted click handler')} > <CheckIcon color="primary" /> </IconButton>src/components/UserListItemCard/LandscapeCard.tsx (4)
20-26: LGTM! Theme-aware Card styling.The migration to
sxprop with theme function is correct. The relative positioning properly supports the absolutely positioned pin button child.
28-33: LGTM! Proper flex layout styling.The CardContent styling correctly implements horizontal layout with vertical alignment using the
sxprop.
34-43: LGTM! Theme-aware Avatar styling.The Avatar styling properly uses the
squarehelper and theme values for consistent sizing and spacing. The migration correctly leverages the Avatar component's sx support.
68-72: LGTM! Proper absolute positioning.The TogglePinButton styling correctly uses absolute positioning to overlay the button on the card, working in conjunction with the Card's relative positioning.
src/components/form/EnumField.tsx (2)
241-248: Clean migration to sx-based conditional styling.The conditional styling for the
toggle-splitvariant correctly applies negative margin with theme-based padding. The pattern follows MUI best practices.
257-259: Correct sx prop migration.The theme-based margin styling is correctly implemented using the standard MUI pattern.
src/components/Error/Error.tsx (4)
3-3: LGTM: useTheme import added for theme-aware inline styling.The import is necessary to access
theme.spacing()for the inline style implementation.
56-56: LGTM: Theme hook correctly initialized.The theme instance is properly accessed for spacing calculations in the inline styles below.
80-87: LGTM: Inline style correctly replaces makeStyles for conditional page styling.The use of the
styleprop (rather thansx) is appropriate here sinceComponentcan be anyElementType, not necessarily a MUI component. The conditional styling with theme spacing is correctly implemented.
94-94: LGTM: Grid correctly uses sx prop for margin spacing.The
sx={{ mt: 3 }}correctly applies theme-aware margin-top spacing, consistent with the makeStyles migration across the codebase.src/components/files/FileActions/FileVersions.tsx (1)
56-58: LGTM! Clean migration to Box with sx styling.The skeleton loading state now uses Box with inline sx props instead of makeStyles-based styling, maintaining the same padding while simplifying the code.
src/components/files/FileActions/FileActionsMenu.tsx (1)
223-234: LGTM! Box wrapper correctly replaces makeStyles styling.The NewVersion menu item now uses Box with inline sx styling, properly maintaining the dropzone functionality with
getRootProps()and preserving the layout with flex display and compensating margins.src/components/ProjectChangeRequestListItem/ProjectChangeRequestListItem.tsx (3)
1-10: LGTM! Box import added correctly.The
Boximport is properly added to support the new separator implementation on line 49.
34-39: LGTM! Conditional sx logic is correct.The conditional spread pattern correctly applies
display: 'flex'only when data is present. This is a clean migration from the previous className-based approach.
74-80: LGTM! CardActions styling migrated correctly.The sx prop correctly uses the theme function to access spacing values, and the flex layout properties are properly configured for space-between layout.
src/components/MethodologiesCard/MethodologiesCard.tsx (3)
17-21: LGTM! Good addition of className prop.Adding the optional
classNameprop to the interface provides flexibility for parent components to apply custom styles, which is a useful pattern alongside thesxmigration.
60-68: LGTM! CardContent styling migrated correctly.The sx prop correctly translates the layout and spacing requirements using MUI v5 conventions. The use of theme spacing units (
py,px) ensures consistency with the design system.
85-87: LGTM! Card styling approach is sound.The combination of accepting
classNamefrom props while usingsxfor internal layout is a good pattern. This maintains flexibility for parent components while ensuring the card's core layout requirements are met.src/components/PartnerListItemCard/PartnerListItemCard.tsx (2)
34-38: LGTM! Clean migration tosxprop.The styling migration for
CardActionAreaLinkis correct. Since it spreads props toCardActionArea(an MUI component), thesxprop is properly supported.
26-33: LGTM!TogglePinButtoncorrectly supportssxprop.The styling migration is verified and correct:
TogglePinButtonextendsIconButtonProps(excluding 'children'), which includes thesxprop- The component spreads remaining props via
...rest, properly forwardingsxto the underlying button- Card uses responsive width with theme breakpoints (idiomatic MUI v5)
- Relative positioning on Card correctly enables absolute positioning of the pin button
src/scenes/Languages/Detail/LanguageDetail.tsx (4)
79-88: Clean migration to Box with sx props.The main container styling is well-structured. The use of the
'& > *:not(:last-child)'selector pattern for child spacing is a clean, idiomatic approach that avoids unnecessary margin on the last element.
98-140: Good use of flexbox with gap for header layout.The flex layout with
gap: 1is cleaner than managing individual margins, and thelineHeight: 'inherit'adjustment for Typography ensures better vertical alignment with the adjacent buttons.
236-239: Visibility logic correctly preserves layout space.The use of
visibility: 'hidden'instead ofdisplay: 'none'maintains consistent layout spacing, which is appropriate for a Fab button that should have stable positioning.
268-270: Both components correctly support the sx prop.LocationCard accepts
StylePropsand applies sx viaextendSx(sx). ProjectListItemCard acceptsStylePropsand applies sx viaextendSx(sx). The code changes passingsx={{ mb: 2 }}are correctly typed and safe.src/components/ProjectBreadcrumb/ProjectBreadcrumb.tsx (1)
36-41: LGTM!The
SensitivityIconstyling migration to thesxprop is clean and follows MUI best practices.src/scenes/Products/List/ProductList.tsx (1)
37-42: LGTM!The migration to the
sxprop is clean and the styling properties are appropriate for the List component layout.src/components/Avatar/Avatar.stories.tsx (1)
10-10: LGTM!Using a fixed
variant="circular"value for the story is acceptable.src/scenes/Engagement/InternshipEngagement/MentorCard/MentorCard.tsx (1)
29-60: LGTM!The
sxprop usage on Typography, CardActionArea, and Avatar components is clean and follows MUI best practices. The theme callback for Avatar styling is particularly well done.src/components/Avatar/Avatar.tsx (1)
11-30: LGTM!The migration from makeStyles to inline
sxstyling is well executed. The conditional logic for the loading state properly handles prop spreading and the Skeleton styling is appropriate.src/components/MemberListSummary/MemberListSummary.tsx (1)
59-80: LGTM!The
sxprop usage on AvatarGroup and Typography components is clean and follows MUI best practices.src/components/LanguageEngagementListItemCard/LanguageEngagementListItemCard.tsx (2)
37-52: LGTM!The migration from makeStyles to
sxprops on Card, CardActionAreaLink, and CardContent is clean and follows MUI best practices. The flex layout properties are appropriate.
84-101: LGTM!The use of the
Stackcomponent withsxstyling for right-aligned content is a good choice. The flex properties and spacing are appropriate for the layout.src/components/posts/PostList.tsx (1)
44-46: LGTM!The migration to sx-based styling via
ContainerPropsis clean and maintains the same layout constraint.src/components/PartnershipCard/PartnershipPrimaryIcon.tsx (1)
5-16: LGTM!The migration to sx-based styling is well-implemented. Using
extendSxto merge incomingsxprops with defaults is the correct pattern for this type of refactor.src/components/PartnershipCard/PartnershipCard.tsx (1)
32-36: LGTM!The styling migration to sx props is clean and consistent throughout the component. The removal of
classNamefrom the public API aligns with the PR objectives.Also applies to: 106-112
src/components/UserListItemCard/PortraitCard.tsx (1)
29-66: LGTM!The migration to sx-based styling is thorough and consistent. The use of theme callbacks for responsive sizing is appropriate.
Note:
classNameis still present in the props interface (line 16) but is no longer used in the component. Consider removing it in a follow-up if it's no longer part of the public API.src/components/List/List.tsx (1)
33-35: LGTM!The migration to sx-based styling is well-executed. The component correctly:
- Adds
StylePropsto the public API for external customization- Uses
extendSxto properly merge incomingsxprops with default styles- Replaces the previous
makeStylesapproach with inlinesxpropsThis is a good example of the migration pattern for this PR.
Also applies to: 61-71
src/components/Icons/HugeIcon.stories.tsx (1)
14-14: LGTM!The migration from
makeStylesto inlinesxprop is straightforward and correct.src/components/Changeset/ChangesetBanner.tsx (1)
23-26: LGTM!The migration to
sxprop styling is clean and correct. The theme callback properly accesses breakpoints and spacing values.src/components/Icons/HugeIcon.tsx (1)
25-34: LGTM!The sx-based styling correctly applies fixed dimensions and colors. Spreading
rest.sxat the end properly allows consumers to override styles when needed.src/scenes/Projects/ChangeRequest/List/ProjectChangeRequestList.tsx (1)
27-90: LGTM!The Box-based layout with sx props is well-structured. The theme callbacks properly apply responsive constraints and spacing, maintaining the same visual behavior as the previous makeStyles implementation.
src/scenes/Projects/Members/List/ProjectMembersList.tsx (1)
37-126: LGTM!The Box-based layout migration is consistent with the broader refactoring pattern. Theme callbacks and sx props are properly used throughout.
src/components/FieldOverviewCard/FieldOverviewCard.tsx (1)
57-155: LGTM!The sx-based styling migration is comprehensive and correct. The conditional styling for empty values (lines 92-97) properly uses sx with theme callbacks. The Box component (lines 77-86) cleanly replaces the previous className-based div.
src/components/LanguageListItemCard/LanguageListItemCard.tsx (1)
29-131: Styling migration looks good overall.The Card, Typography, Sensitivity, and TogglePinButton components all properly use sx props with appropriate theme-based values.
src/components/Changeset/PropertyDiff.tsx (1)
22-48: LGTM! Excellent use of the extendSx pattern.The migration properly uses the
extendSxutility (lines 22-27) to compose sx props, allowing consumers to extend the base styles. The theme callbacks correctly access palette, spacing, and shape values for consistent styling.src/scenes/Projects/Overview/ProjectOverview.tsx (1)
149-578: LGTM! Comprehensive and well-structured migration.The Box-based layout with sx props is thorough and correctly implemented throughout this complex component. Notable highlights:
- Proper use of theme callbacks for responsive constraints (lines 159-160)
- Child selectors for spacing (lines 161-163) work correctly with MUI sx
- Conditional styling in Typography (lines 175-189) properly handles loading states
- The negative margin adjustment (line 538) and ContainerProps usage (lines 541-543) correctly fine-tune the List spacing
src/components/LocationCard/LocationCard.tsx (4)
9-9: Migration to StyleProps looks good.The addition of
extendSxandStylePropsimports and the updated component signature properly support the newsx-based styling approach.Also applies to: 28-29
32-32: Correct use of extendSx pattern.The
sxprop correctly merges base styles with external styles using the spread operator andextendSxhelper.
61-61: Inline sx styling applied correctly.
78-80: Correct use of MUI spacing shorthand.src/components/ProjectListItemCard/ProjectListItemCard.tsx (7)
2-2: Import and signature updates are correct.The additions of
Box,extendSx, andStylePropsproperly support the migration tosx-based styling.Also applies to: 13-13, 29-30
34-43: Good use of theme-aware styling.The
sxprop correctly uses a theme callback to access breakpoint values and properly merges external styles withextendSx.
47-47: Flexbox layout styling applied correctly.Both
CardActionAreaLinkandCardContentuse appropriatesxprops for the card layout with proper theme spacing.Also applies to: 49-57
61-66: Layout and spacing styles are correct.Also applies to: 101-103
122-131: Box styling is correct for the right content layout.
145-145: Skeleton styling migrated correctly.Also applies to: 156-160
195-199: Pin button positioning is correct.src/components/OrganizationListItemCard/OrganizationListItemCard.tsx (3)
26-30: LGTM!The
sxprop onCardActionAreaLinkis correctly applied. The flex display withalignItems: 'initial'provides appropriate layout control for the card's action area.
31-39: LGTM!The
sxstyling onCardContentis well-structured with appropriate flex layout properties and spacing values. The use of MUI's spacing shorthand (py,px) is idiomatic and maintains consistency with the theme.
6-9: Confirm whetherclassNameshould be removed, migrated toStyleProps, or retained for backwards compatibility.Verification shows this component retains an explicit
classNameprop while using the modernsxprop pattern, but similar components are inconsistent:ProjectListItemCardhas noclassNameprop, whilePartnerListItemCardretains it. The modern migration pattern used by other updated components extendsStyleProps(which provides bothsxandclassNameautomatically) rather than manually declaringclassName. Please confirm if this component should adopt theStylePropspattern for consistency with components likeProgressReportsOverviewCardandPeriodicReportCard, or if retention is intentional.
| <Grid | ||
| item | ||
| wrap="nowrap" | ||
| sx={{ display: 'flex', color: 'text.secondary', mr: 0.5 }} | ||
| > |
There was a problem hiding this comment.
Remove invalid wrap prop from Grid item.
The wrap prop is only valid for Grid containers (components with the container prop), not Grid items. MUI will ignore this prop, but it should be removed to avoid confusion.
Apply this diff to remove the invalid prop:
<Grid
item
- wrap="nowrap"
sx={{ display: 'flex', color: 'text.secondary', mr: 0.5 }}
>📝 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.
| <Grid | |
| item | |
| wrap="nowrap" | |
| sx={{ display: 'flex', color: 'text.secondary', mr: 0.5 }} | |
| > | |
| <Grid | |
| item | |
| sx={{ display: 'flex', color: 'text.secondary', mr: 0.5 }} | |
| > |
🤖 Prompt for AI Agents
In src/components/MethodologiesCard/MethodologiesCard.tsx around lines 40 to 44,
the Grid item is using the invalid wrap prop (wrap is only valid on Grid
containers); remove the wrap="nowrap" attribute from this Grid item so it only
uses valid props (e.g., item, sx, color) to avoid confusion and rely on
container-level wrap when needed.
| <div | ||
| style={{ | ||
| flex: 2, | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| justifyContent: 'center', | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use sx instead of inline style for consistency.
This is the only element in the file using inline style instead of sx. For consistency with the styling migration and to maintain theme integration, consider converting to sx or using a Box component.
Apply this diff to use sx:
- <div
- style={{
- flex: 2,
- display: 'flex',
- flexDirection: 'column',
- justifyContent: 'center',
- }}
- >
+ <Box
+ sx={{
+ flex: 2,
+ display: 'flex',
+ flexDirection: 'column',
+ justifyContent: 'center',
+ }}
+ >
<Typography variant="h1">
{!project ? (
<Skeleton
variant="text"
sx={{ marginLeft: 'auto', width: '1ch' }}
/>
) : (
project.engagements.total
)}
</Typography>
<Typography variant="body2" color="primary">
{!project ? (
<>
<Skeleton
variant="text"
sx={{ marginLeft: 'auto', width: '9ch' }}
/>
<Skeleton
variant="text"
sx={{ marginLeft: 'auto', width: '11ch' }}
/>
</>
) : (
<>
{project.type === 'Internship' ? 'Internship' : 'Language'}
<br />
Engagements
</>
)}
</Typography>
- </div>
+ </Box>📝 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.
| <div | |
| style={{ | |
| flex: 2, | |
| display: 'flex', | |
| flexDirection: 'column', | |
| justifyContent: 'center', | |
| }} | |
| > | |
| <Box | |
| sx={{ | |
| flex: 2, | |
| display: 'flex', | |
| flexDirection: 'column', | |
| justifyContent: 'center', | |
| }} | |
| > |
🤖 Prompt for AI Agents
In src/components/ProjectListItemCard/ProjectListItemCard.tsx around lines 133
to 140, replace the inline style prop on the div with an MUI sx prop (or convert
the element to a Box) so styling is consistent with the rest of the file and
theme-aware; specifically remove the style={{ flex: 2, display: 'flex',
flexDirection: 'column', justifyContent: 'center' }} and apply equivalent sx={{
flex: 2, display: 'flex', flexDirection: 'column', justifyContent: 'center' }}
(or use <Box sx=...>), ensuring imports for Box are added if you switch to Box
and tests/formatting are updated.
| export const Sensitivity = ({ | ||
| value, | ||
| loading, | ||
| className, | ||
| }: SensitivityProps) => { | ||
| const { classes, cx } = useStyles(); | ||
| sx, | ||
| }: SensitivityProps & StyleProps) => { | ||
| const theme = useTheme(); | ||
|
|
||
| const sensitivityColors = { | ||
| Low: grey[400], | ||
| Medium: theme.palette.warning.main, | ||
| High: theme.palette.error.main, | ||
| } as const; | ||
|
|
||
| return ( | ||
| <div className={className}> | ||
| <div className={classes.iconWrapper}> | ||
| <VerifiedUserOutlined className={classes.icon} /> | ||
| <Box sx={extendSx(sx)}> | ||
| <Box | ||
| sx={{ |
There was a problem hiding this comment.
Restore className pass-through
We still accept StyleProps, so callers can supply className, but the destructuring here omits it and the outer <Box> no longer forwards it. That regresses any consumers relying on the class for styling overrides. Please keep the prop and pass it through.
-export const Sensitivity = ({
- value,
- loading,
- sx,
-}: SensitivityProps & StyleProps) => {
+export const Sensitivity = ({
+ value,
+ loading,
+ sx,
+ className,
+}: SensitivityProps & StyleProps) => {
…
- return (
- <Box sx={extendSx(sx)}>
+ return (
+ <Box className={className} sx={extendSx(sx)}>📝 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.
| export const Sensitivity = ({ | |
| value, | |
| loading, | |
| className, | |
| }: SensitivityProps) => { | |
| const { classes, cx } = useStyles(); | |
| sx, | |
| }: SensitivityProps & StyleProps) => { | |
| const theme = useTheme(); | |
| const sensitivityColors = { | |
| Low: grey[400], | |
| Medium: theme.palette.warning.main, | |
| High: theme.palette.error.main, | |
| } as const; | |
| return ( | |
| <div className={className}> | |
| <div className={classes.iconWrapper}> | |
| <VerifiedUserOutlined className={classes.icon} /> | |
| <Box sx={extendSx(sx)}> | |
| <Box | |
| sx={{ | |
| export const Sensitivity = ({ | |
| value, | |
| loading, | |
| sx, | |
| className, | |
| }: SensitivityProps & StyleProps) => { | |
| const theme = useTheme(); | |
| const sensitivityColors = { | |
| Low: grey[400], | |
| Medium: theme.palette.warning.main, | |
| High: theme.palette.error.main, | |
| } as const; | |
| return ( | |
| <Box className={className} sx={extendSx(sx)}> | |
| <Box | |
| sx={{ |
🤖 Prompt for AI Agents
In src/components/Sensitivity/Sensitivity.tsx around lines 17 to 33, the
component destructures value, loading, and sx but drops className from
StyleProps and therefore the outer <Box> no longer forwards it; restore
className in the destructuring and pass it through to the outer Box (e.g.,
include className in the props and add it as a prop on the outer Box alongside
sx) so callers can continue to supply class-based styling overrides.
There was a problem hiding this comment.
@rdonigian I kinda agree, but won't make you do this everywhere in this PR.
className being the smallest primitive seems like a miss to not accept it.
Also FWIW using css prop from Emotion is transpiled into className prop.
So tooling currently allows
<Sensitivity css={{ m: 1 }} />and that will be converted to a passed className.
That's why StyleProps includes the className prop in addition to sx.
This could be better documented.
There is an pitfall when using sx & emotion directly and trying to "override" a certain css property. That's why we don't mix it with MUI components.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
CarsonF
left a comment
CarsonF
left a comment
There was a problem hiding this comment.
Huge @rdonigian! Thank you for tackling!
| colorDefault: loading ? classes.loading : props.classes?.colorDefault, | ||
| sx={{ | ||
| ...(loading && { backgroundColor: 'transparent' }), | ||
| ...props.sx, |
There was a problem hiding this comment.
props.sx has to be merged like this
sx={[
{ ... }, // our default styles
...extendSx(props.sx),
]}Because sx is
Many<StyleObject | ((Theme) => StyleObject)so we can't just object spread naively.
| <> | ||
| {leftAction} | ||
| <div className={classes.spacer} /> | ||
| <div style={{ flex: 1 }} /> |
There was a problem hiding this comment.
Minor
| <div style={{ flex: 1 }} /> | |
| <Box sx={{ flex: 1 }} /> |
| height: 64, | ||
| color: 'info.main', | ||
| backgroundColor: 'grey.200', | ||
| ...rest.sx, |
| LinkProps={{ | ||
| underline: data?.name.canRead ? undefined : 'none', | ||
| }} | ||
| style={{ display: 'flex', alignItems: 'center', ...rest.style }} |
| export const Sensitivity = ({ | ||
| value, | ||
| loading, | ||
| className, | ||
| }: SensitivityProps) => { | ||
| const { classes, cx } = useStyles(); | ||
| sx, | ||
| }: SensitivityProps & StyleProps) => { | ||
| const theme = useTheme(); | ||
|
|
||
| const sensitivityColors = { | ||
| Low: grey[400], | ||
| Medium: theme.palette.warning.main, | ||
| High: theme.palette.error.main, | ||
| } as const; | ||
|
|
||
| return ( | ||
| <div className={className}> | ||
| <div className={classes.iconWrapper}> | ||
| <VerifiedUserOutlined className={classes.icon} /> | ||
| <Box sx={extendSx(sx)}> | ||
| <Box | ||
| sx={{ |
There was a problem hiding this comment.
@rdonigian I kinda agree, but won't make you do this everywhere in this PR.
className being the smallest primitive seems like a miss to not accept it.
Also FWIW using css prop from Emotion is transpiled into className prop.
So tooling currently allows
<Sensitivity css={{ m: 1 }} />and that will be converted to a passed className.
That's why StyleProps includes the className prop in addition to sx.
This could be better documented.
There is an pitfall when using sx & emotion directly and trying to "override" a certain css property. That's why we don't mix it with MUI components.
| width: `calc(100% + (${spacing(2)} * 2))`, | ||
| '&:focus': { | ||
| outline: 'none', | ||
| }, |
| <ListItemText | ||
| onClick={() => openFilePreview(version)} | ||
| className={classes.text} | ||
| sx={{ cursor: 'pointer', marginRight: 3 }} |
There was a problem hiding this comment.
Just FYI throwing cursor pointer & an onClick handler on a random element is not good practice. No change needed here - I just wanted to call it out.
| textAlign: 'right', | ||
| }, | ||
| }), | ||
| ...props.InputProps?.sx, |
| ContainerProps={{ | ||
| sx: { maxWidth: 600 }, | ||
| }} |
There was a problem hiding this comment.
Idk if this is needed but technically, I think, input could include this so this would be dropping that input.
ContainerProps={{
...rest.ContainerProps,
sx: [{ maxWidth: 600 }, ...extendSx(rest.ContainerProps)]
}}| // To match certification title. | ||
| // This is hacky, we should try to find a way to do this without pixel | ||
| // coupling. | ||
| marginTop: 7, | ||
| marginBottom: 15, |
There was a problem hiding this comment.
These are raw pixel values, but in sx they are x8.
In sx you need { mt: '7px' }.
Also can you restore comment in new impl?
No description provided.