Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XS][1648] Add noWrap to team name display in TeamCharacterSelector #2518

Merged
merged 5 commits into from
Oct 27, 2024
Merged

[XS][1648] Add noWrap to team name display in TeamCharacterSelector #2518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d3164f
init
BaruYogesh Oct 24, 2024
bfd232a
hier
BaruYogesh Oct 24, 2024
4dacef1
more spots
BaruYogesh Oct 24, 2024
f2ec382
build name
BaruYogesh Oct 26, 2024
be4568f
buildcard
BaruYogesh Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions libs/gi/page-team/src/CharacterDisplay/Content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,22 +173,34 @@ function TabNav({
'&:hover': {
color: 'info.light',
},
alignItems: 'center',
}}
>
<PersonIcon />
<Typography
variant="h6"
noWrap
sx={{
display: 'flex',
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
}}
>
<PersonIcon />
<strong>{teamChar.name}</strong>
<Divider orientation="vertical" variant="middle" flexItem />
<CheckroomIcon />
</Typography>
Comment on lines +179 to +191
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add maxWidth and text overflow handling for character names

The Typography component with noWrap lacks maxWidth constraints and text overflow handling, which could lead to layout issues with long character names. Add the following properties to the Typography's sx prop:

  • maxWidth
  • textOverflow: 'ellipsis'
  • overflow: 'hidden'
🔗 Analysis chain

Verify text overflow handling for long character names

The addition of noWrap prevents text wrapping as intended. However, we should ensure proper text truncation for long character names.

Let's verify the text overflow behavior:

Let me check the surrounding context of the Typography component to better understand the layout constraints.

Let me try a different approach to check the component's structure and styling.

Let me check the parent container's styling to understand the width constraints.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any maxWidth constraints or text overflow handling in the codebase
rg -g '*.tsx' -g '*.ts' 'textOverflow|maxWidth.*Typography' ./libs/gi/page-team/

Length of output: 166

Script:

#!/bin/bash
# Check the parent container's styling and width constraints
rg -g '*.tsx' -A 10 -B 10 '<Typography.*variant="h6".*noWrap' ./libs/gi/page-team/src/

Length of output: 88

Script:

#!/bin/bash
# Get the full content of CharacterDisplay/Content.tsx to understand the layout context
rg -g 'Content.tsx' -A 5 -B 5 'PersonIcon' ./libs/gi/page-team/src/CharacterDisplay/

Length of output: 2880

Script:

#!/bin/bash
# Get more context around the Typography component to see the parent container
rg -g 'Content.tsx' -A 20 -B 20 'variant="h6".*noWrap' ./libs/gi/page-team/src/CharacterDisplay/

Length of output: 96

<Divider orientation="vertical" variant="middle" flexItem />
<CheckroomIcon />
<Typography
variant="h6"
noWrap
sx={{
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
}}
>
Comment on lines +194 to +203
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider flexible width distribution for long names

While the noWrap property prevents text wrapping as intended, consider adding flexible width distribution between character name and build name to handle cases where both are long.

Consider applying this improvement:

 <Typography
   variant="h6"
   noWrap
   sx={{
     gap: 1,
     alignItems: 'center',
     justifyContent: 'center',
     textShadow: '#000 0 0 10px !important',
+    flexShrink: 1,
+    minWidth: 0,
+    overflow: 'hidden',
+    textOverflow: 'ellipsis'
   }}
 >

This change would:

  1. Allow the Typography to shrink when space is limited
  2. Ensure text truncation with ellipsis when content overflows
  3. Maintain the desired single-line display
📝 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.

Suggested change
<Typography
variant="h6"
noWrap
sx={{
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
}}
>
<Typography
variant="h6"
noWrap
sx={{
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
flexShrink: 1,
minWidth: 0,
overflow: 'hidden',
textOverflow: 'ellipsis'
}}
>

{database.teams.getActiveBuildName(loadoutDatum)}
</Typography>
</CardContent>
Expand Down
4 changes: 3 additions & 1 deletion libs/gi/page-team/src/LoadoutDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,12 @@ export function LoadoutDropdown({
title={
<Box
sx={{
display: 'flex',
gap: 1,
flexWrap: 'wrap',
justifyContent: 'center',
textOverflow: 'ellipsis',
overflow: 'hidden',
whiteSpace: 'nowrap',
}}
>
{label ? (
Expand Down
24 changes: 12 additions & 12 deletions libs/gi/page-team/src/TeamCharacterSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,13 @@ export default function TeamCharacterSelector({
placement="top"
title={
<Box>
<Box sx={{ display: 'flex', color: 'info.light', gap: 1 }}>
<Box
sx={{
display: 'flex',
color: 'info.light',
gap: 1,
}}
>
<BorderColorIcon />
<Typography>
<strong>{t`team.editNameDesc`}</strong>
Expand All @@ -139,23 +145,17 @@ export default function TeamCharacterSelector({
<CardContent
sx={{
display: 'flex',
alignItems: 'center',
justifyContent: 'center',
'&:hover': {
color: 'info.light',
},
gap: 1,
textShadow: '#000 0 0 10px !important',
}}
>
<Typography
variant="h5"
sx={{
display: 'flex',
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
}}
>
<TeamIcon />
<TeamIcon />
<Typography noWrap variant="h5">
{team.name}
</Typography>
</CardContent>
Expand Down
6 changes: 5 additions & 1 deletion libs/gi/ui/src/components/build/BuildCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ export function BuildCard({
const clickableAreaContent = (
<>
<CardHeader
title={name}
title={
<Typography noWrap gutterBottom variant="h6">
{name}
</Typography>
}
Comment on lines +49 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding a title attribute for accessibility.

When text is truncated with noWrap, the full text should be accessible via tooltip for users who need to see the complete build name.

 <Typography noWrap gutterBottom variant="h6"
+           title={typeof name === 'string' ? name : undefined}
 >
   {name}
 </Typography>
📝 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.

Suggested change
title={
<Typography noWrap gutterBottom variant="h6">
{name}
</Typography>
}
title={
<Typography noWrap gutterBottom variant="h6"
title={typeof name === 'string' ? name : undefined}
>
{name}
</Typography>
}

action={
description && (
<Tooltip title={<Typography>{description}</Typography>}>
Expand Down
Loading