Make header buttons more button-like #2578
Conversation
WalkthroughThis pull request introduces comprehensive updates across multiple files in the Genshin Impact (GI) page team module. The changes primarily focus on enhancing UI components, theme configuration, and component interactions. The modifications include extending the theme's color palette, updating component styling, replacing Changes
Possibly Related PRs
Poem
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 (
|
|
[sr-frontend] [Mon Dec 16 09:06:01 UTC 2024] - Deployed ae3e850 to https://genshin-optimizer-prs.github.io/pr/2578/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Dec 16 09:06:12 UTC 2024] - Deployed ae3e850 to https://genshin-optimizer-prs.github.io/pr/2578/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Dec 16 09:39:00 UTC 2024] - Deployed 107586e to https://genshin-optimizer-prs.github.io/pr/2578/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Dec 16 09:39:33 UTC 2024] - Deployed 107586e to https://genshin-optimizer-prs.github.io/pr/2578/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Dec 16 20:57:38 UTC 2024] - Deployed ef9ff3f to https://genshin-optimizer-prs.github.io/pr/2578/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Mon Dec 16 20:58:14 UTC 2024] - Deployed ef9ff3f to https://genshin-optimizer-prs.github.io/pr/2578/sr-frontend (Takes 3-5 minutes after this completes to be available) [Wed Dec 18 03:06:49 UTC 2024] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
libs/common/ui/src/theme/index.ts (1)
Line range hint
227-237: Fix inconsistent naming in neutral color definitions.Some neutral colors are incorrectly named as 'primary' which could cause confusion.
neutral700: defaultTheme.palette.augmentColor({ color: { main: '#191C2B' }, - name: 'primary', + name: 'neutral700', }), neutral800: defaultTheme.palette.augmentColor({ color: { main: '#0C0F1A' }, - name: 'primary', + name: 'neutral800', }), neutral900: defaultTheme.palette.augmentColor({ color: { main: '#020515' }, - name: 'primary', + name: 'neutral900', }),
🧹 Nitpick comments (4)
libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx (1)
51-57: Simplify Typography stylingThe Typography component inside the Button has unnecessary styling properties. The Button component already handles alignment of its children.
<Typography variant="h6" noWrap - sx={{ - alignItems: 'center', - justifyContent: 'center', - }} >libs/gi/page-team/src/CharacterDisplay/Content.tsx (1)
130-134: Consider memoizing color calculationsThe color calculations are performed on every render. Consider memoizing these values using useMemo to optimize performance.
+ const colorMemo = useMemo(() => { const color = elementKey && theme.palette[elementKey]?.main const colorrgb = color && hexToColor(color) return (alpha = 1) => (colorrgb && colorToRgbaString(colorrgb, alpha)) ?? `rgba(255,255,255,${alpha})` + }, [elementKey, theme])libs/gi/page-team/src/TeamCharacterSelector.tsx (1)
225-277: Consider adding form validationThe TeamEditorModal component could benefit from form validation to ensure data quality. Consider adding validation for empty names or maximum length constraints.
const handleName = (teamName: string): void => { + if (!teamName.trim()) return + if (teamName.length > 50) return database.teams.set(teamId, { name: teamName }) }libs/gi/page-team/src/index.tsx (1)
Line range hint
171-190: Nice touch with the element-based gradient background!The dynamic background gradient based on character's element adds visual depth and reinforces the game's element system. However, consider caching the gradient value to optimize performance.
sx={(theme) => { const elementKey = characterKey && getCharEle(characterKey) if (!elementKey) return {} const hex = theme.palette[elementKey].main as string const color = hexToColor(hex) if (!color) return {} const rgba = colorToRgbaString(color, 0.1) + // Memoize the gradient value using useMemo to prevent recalculation on every render + const gradientStyle = useMemo(() => ({ + background: `linear-gradient(to bottom, ${rgba} 0%, rgba(0,0,0,0)) 25%` + }), [rgba]) - return { - background: `linear-gradient(to bottom, ${rgba} 0%, rgba(0,0,0,0)) 25%` - } + return gradientStyle }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/common/ui/src/theme/index.ts(1 hunks)libs/gi/page-team/src/CharacterDisplay/Content.tsx(9 hunks)libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx(2 hunks)libs/gi/page-team/src/TeamCharacterSelector.tsx(8 hunks)libs/gi/page-team/src/index.tsx(2 hunks)
🔇 Additional comments (7)
libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx (2)
3-3: LGTM! Clean component signature update
The addition of ElementKey type and Button component, along with the updated component signature, provides better type safety and aligns with the goal of making headers more button-like.
Also applies to: 8-8, 14-14
64-96: LGTM! Consistent button implementation
The second button maintains consistency with the first button's styling and behavior, creating a cohesive user interface.
libs/gi/page-team/src/CharacterDisplay/Content.tsx (1)
Line range hint 88-98: LGTM! Improved visual hierarchy
The adjustments to box shadow (1px) and background opacity (0.3) enhance the visual hierarchy while maintaining readability.
libs/gi/page-team/src/TeamCharacterSelector.tsx (2)
63-63: LGTM! Clean state management
The use of useBoolState hook simplifies modal state management and reduces boilerplate code.
Line range hint 172-201: LGTM! Consistent tab styling
The tab styling maintains consistency with the button-like design pattern, creating a cohesive user interface.
libs/gi/page-team/src/index.tsx (1)
203-212: LGTM! Clean implementation of flexible container.
The Box component with consistent padding and gap provides a better structure for the content layout.
libs/common/ui/src/theme/index.ts (1)
106-115: LGTM! Comprehensive neutral color palette addition.
The neutral color palette provides a good range of options for button styling, improving the visual hierarchy of the UI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx (2)
23-55: Consider extracting common tooltip styles and enhancing accessibilityWhile the implementation is solid, consider these improvements:
- The tooltip style structure is duplicated. Consider extracting it to a reusable component.
- Add
aria-labelto improve accessibility for screen readers.Example refactor:
+ const TooltipContent = ({ title }: { title: string }) => ( + <Box> + <Box sx={{ display: 'flex', color: 'info.light', gap: 1 }}> + <BorderColorIcon /> + <Typography> + <strong>{title}</strong> + </Typography> + </Box> + {!!teamChar.description && ( + <Typography>{teamChar.description}</Typography> + )} + </Box> + ) <BootstrapTooltip placement="top" - title={ - <Box> - <Box sx={{ display: 'flex', color: 'info.light', gap: 1 }}> - <BorderColorIcon /> - <Typography> - <strong>{t('loadout.edit')}</strong> - </Typography> - </Box> - {!!teamChar.description && ( - <Typography>{teamChar.description}</Typography> - )} - </Box> - } + title={<TooltipContent title={t('loadout.edit')} />} > <Button + aria-label={`Edit loadout for ${teamChar.name}`} // ... rest of the button props >
Line range hint
14-93: Good architectural choice moving to semantic button elementsThe transition from
CardActionAreato semanticButtoncomponents is a solid architectural decision that:
- Improves accessibility by using proper semantic HTML
- Makes the interactive nature of these elements more obvious to users
- Provides better keyboard navigation support by default
The use of
elementKeyfor styling is also a good pattern for maintaining visual consistency with the game's element system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx(2 hunks)
🔇 Additional comments (3)
libs/gi/page-team/src/CharacterDisplay/LoadoutHeader.tsx (3)
3-8: LGTM! Import changes align with button-centric UI approach
The removal of Card-related imports and addition of Button components aligns well with the PR's objective of making headers more button-like.
Line range hint 14-20: LGTM! Clean component signature with proper type safety
The addition of the elementKey prop with proper TypeScript typing enhances the component's flexibility for element-based styling. Hook usage follows React best practices.
57-89: Consider handling long build names and text overflow
While the Typography component has noWrap, there might be cases where very long build names could affect the layout. Consider:
- Adding a maximum width constraint
- Implementing text truncation with ellipsis
- Adding a tooltip with the full name for truncated text
Let's check for potentially long build names in the database:
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
libs/gi/ui/src/components/character/CharacterCard.tsx (1)
Line range hint
52-59: Consider replacing CardActionArea with Button componentGiven the PR's objective of making elements more button-like, consider using MUI's Button component instead of CardActionArea. This would provide better semantic meaning and accessibility.
- <CardActionArea - onClick={onClickHandler} - sx={{ flexGrow: 1, display: 'flex', flexDirection: 'column' }} - > - {children} - </CardActionArea> + <Button + onClick={onClickHandler} + variant="outlined" + fullWidth + sx={{ + flexGrow: 1, + display: 'flex', + flexDirection: 'column', + textTransform: 'none' + }} + > + {children} + </Button>
🧹 Nitpick comments (7)
libs/gi/page-team/src/TeamSetting/TeamComponents.tsx (1)
160-174: Consider performance and maintainability improvements.The element-based styling implementation looks good, but could benefit from some refinements:
- Consider extracting magic numbers for opacity into named constants
- The color transformation could be memoized to prevent unnecessary recalculations
Here's a suggested improvement:
+ const BORDER_OPACITY = { + default: 0.3, + hover: 0.8, + } as const; + <CardThemed bgt="light" sx={(theme) => { + const getColorRgba = useMemo(() => { const color = elementKey && theme.palette[elementKey]?.main const colorrgb = color && hexToColor(color) return (alpha = 1) => (colorrgb && colorToRgbaString(colorrgb, alpha)) ?? `rgba(255,255,255,${alpha})` + }, [elementKey, theme.palette]); return { - border: `1px solid ${colorrbga(0.3)}`, + border: `1px solid ${getColorRgba(BORDER_OPACITY.default)}`, '&:hover': { - border: `1px solid ${colorrbga(0.8)}`, + border: `1px solid ${getColorRgba(BORDER_OPACITY.hover)}`, }, } }} >libs/gi/ui/src/components/character/CharacterCard.tsx (1)
93-101: Use theme values for consistent stylingWhile the border and hover effects align with making elements more interactive, consider using theme values for colors and borders instead of hardcoded rgba values.
sx={{ height: '100%', display: 'flex', flexDirection: 'column', - border: '1px solid rgba(200,200,200,0.3)', - ':hover': { - border: '1px solid rgba(200,200,200,0.8)', - }, + border: `1px solid ${theme.palette.divider}`, + ':hover': { + border: `1px solid ${theme.palette.action.hover}`, + }, }}libs/gi/ui/src/components/artifact/ArtifactCardNano.tsx (2)
Line range hint
41-48: Consider converting CardActionArea to an outlined button for consistency.Given the PR's objective of making interactive elements more button-like, consider updating the
CardActionAreawrapper to use an outlined button style, similar to other changes in this PR. This would make the interactive nature of the artifact card more obvious to users.Example approach:
- const actionWrapperFunc = useCallback( - (children: ReactNode) => ( - <CardActionArea onClick={onClick} sx={{ height: '100%' }}> - {children} - </CardActionArea> - ), - [onClick] - ) + const actionWrapperFunc = useCallback( + (children: ReactNode) => ( + <Button + variant="outlined" + onClick={onClick} + sx={{ + height: '100%', + p: 0, + '&:hover': { + backgroundColor: 'transparent' + } + }} + > + {children} + </Button> + ), + [onClick] + )
Line range hint
134-157: Enhance accessibility for artifact information.The tooltips and interactive elements could benefit from improved accessibility:
- Add descriptive ARIA labels to the interactive elements
- Enhance tooltip content with more context
- Ensure color contrast meets WCAG guidelines for stat values
Example improvements:
<BootstrapTooltip placement="top" title={ <Typography> - <StatColoredWithUnit statKey={mainStatKey} /> + {`Main Stat: ${mainStatKey}`} + <StatColoredWithUnit statKey={mainStatKey} /> </Typography> } + aria-label={`View details for ${mainStatKey}`} disableInteractive > <Box lineHeight={0}> <StatIcon statKey={mainStatKey} + aria-hidden="true" iconProps={{ style: { padding: '4px' } }} /> </Box> </BootstrapTooltip>libs/gi/ui/src/components/build/BuildCard.tsx (1)
80-87: Consider standardizing button color across componentsThe button uses
color="neutral300"while LoadoutCard usesneutral100. Consider standardizing this for consistency across the application.libs/gi/ui/src/components/team/TeamDelModal.tsx (1)
Line range hint
47-47: Fix typo in state setter nameThe state setter name contains a typo:
setDelArryshould besetDelArr.- const [delArr, setDelArry] = useState(() => + const [delArr, setDelArr] = useState(() =>libs/gi/ui/src/components/team/TeamCard.tsx (1)
128-140: Consider adding an aria-label to the delete button.The delete button currently only has an icon, which might not be clear to screen readers.
Apply this diff:
<Button variant="outlined" size="small" onClick={onShowDel} color="error" + aria-label="Delete team" sx={{ borderTopLeftRadius: 0, borderBottomRightRadius: 0, borderBottomLeftRadius: 0, }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/gi/page-team/src/TeamSetting/TeamComponents.tsx(4 hunks)libs/gi/ui/src/components/artifact/ArtifactCardNano.tsx(1 hunks)libs/gi/ui/src/components/build/BuildCard.tsx(4 hunks)libs/gi/ui/src/components/character/CharacterCard.tsx(3 hunks)libs/gi/ui/src/components/character/editor/LoadoutCard.tsx(2 hunks)libs/gi/ui/src/components/character/editor/LoadoutHeaderContent.tsx(1 hunks)libs/gi/ui/src/components/team/TeamCard.tsx(4 hunks)libs/gi/ui/src/components/team/TeamDelModal.tsx(2 hunks)libs/gi/ui/src/components/weapon/WeaponCardNano.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/gi/ui/src/components/character/editor/LoadoutHeaderContent.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
libs/gi/ui/src/components/team/TeamCard.tsx
[error] 152-160: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (9)
libs/gi/page-team/src/TeamSetting/TeamComponents.tsx (2)
2-2: LGTM: New imports support element-based styling.
The added color manipulation utilities and element retrieval function align well with the PR's objective of enhancing visual representation.
Also applies to: 20-20
150-150: LGTM: Element key retrieval for styling.
The element key is correctly retrieved using the imported getCharEle utility.
libs/gi/ui/src/components/character/CharacterCard.tsx (1)
37-42: LGTM: Props interface restructuring improves code clarity
The inline props definition simplifies the code while maintaining type safety.
libs/gi/ui/src/components/artifact/ArtifactCardNano.tsx (1)
68-68: LGTM! The added padding improves visual balance.
The addition of vertical padding (py: 2) to the empty state Box component helps maintain consistent height and improves the visual presentation when no artifact is selected.
libs/gi/ui/src/components/character/editor/LoadoutCard.tsx (1)
44-57: LGTM! Button implementation enhances visual clarity
The replacement of CardActionArea with Button effectively achieves the PR's goal of making header elements more visually button-like. The styling choices (neutral100 color, outlined variant) maintain subtlety while improving affordance.
libs/gi/ui/src/components/build/BuildCard.tsx (1)
Line range hint 50-67: Verify spacing with Box wrapper
The Box wrapper might affect the content spacing. Please verify that this doesn't create unwanted padding or margins.
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax. Let's try to gather more context about the Box and CardHeader usage patterns in the codebase to verify the spacing concerns.
Let's try without the file type specification since it wasn't recognized. We'll focus on finding similar patterns to understand the spacing conventions in the codebase.
Box wrapper spacing is consistent with codebase patterns
The Box wrapper's spacing implementation in BuildCard.tsx follows established patterns in the codebase:
- The CardContent has explicit spacing control with
sx={{ pt: 0, pb: 1 }}, which is a common pattern seen across multiple components - The Box wrapper without explicit sx props is used similarly in other card components (e.g., in StatFilterCard.tsx, BuildDisplayItem.tsx)
- The current implementation maintains consistent spacing with other card components in the codebase that use similar Box + Card patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Box wrapper patterns to ensure consistency
ast-grep --pattern 'Box>
<CardHeader
$$$
/>'
Length of output: 54
Script:
#!/bin/bash
# Search for Box components wrapping CardHeader with different pattern
rg -U "Box.*\n.*CardHeader" --type tsx
# Search for CardHeader components to compare styling patterns
rg -U "CardHeader.*\n.*sx=\{.*\}" --type tsx
# Search for specific Box+Card styling patterns
rg -U "Box.*sx=\{.*\}.*\n.*Card" --type tsx
Length of output: 217
Script:
#!/bin/bash
# Search for Box components wrapping CardHeader with different pattern
rg -U "Box.*\n.*CardHeader"
# Search for similar Card components to understand spacing patterns
rg -U "CardContent.*sx.*pt:|pb:" -A 2
# Look for other Box wrapper patterns around Card components
rg -U "<Box.*>.*\n.*<Card"
Length of output: 9872
libs/gi/ui/src/components/team/TeamDelModal.tsx (1)
Line range hint 164-180: LGTM! Button implementation maintains consistency
The Button implementation aligns well with the pattern established in other components, maintaining visual consistency across the application.
libs/gi/ui/src/components/weapon/WeaponCardNano.tsx (1)
49-49: LGTM!
The added vertical padding improves the visual spacing around the placeholder weapon type image.
libs/gi/ui/src/components/team/TeamCard.tsx (1)
Line range hint 92-140: LGTM! Button improvements enhance visual clarity.
The replacement of CardActionArea with Button components and the addition of outlined variants make the interactive elements more visually apparent to users.
🧰 Tools
🪛 Biome (1.9.4)
[error] 152-160: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
Describe your changes
Since more functions are folded into the card header modals, it's becoming more imperative to convey to the user that the headers are buttons, without having to hover over them.
Converts CardAction to outlined buttons, where the outline helps user denote that this element is a button.
For consistency, Tabs are outlined as well.
Full changelog:
Issue or discord link
Testing/validation
Current vs New
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Summary by CodeRabbit
New Features
TeamEditorModalcomponent for streamlined team editing functionality.Improvements
LoadoutTabsandCharacterPanelcomponents.LoadoutHeaderto simplify interactions with a button-centric design.TeammateDisplaycomponent with dynamic border colors based on character elements.CharacterCardandLoadoutCardcomponents with updated styling and interaction models.Bug Fixes