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.

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

Fix country selector text overflow issue #8125

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

BKM14
Copy link
Contributor

@BKM14 BKM14 commented Oct 27, 2024

Closes #7906

Modified the two children(TextInputV2 and CountrySelect) in the StyledHalfRowContainer component to always be equal in size and divide the available space equally.
The StyledIconChevronDown component has a flex-shrink: 0 to prevent it from completely disappearing. The same applies for the selectedOption.Icon.
A p tag had to be added to the label to correctly handle the text overflow.

@Devessier Devessier self-assigned this Oct 28, 2024
@BKM14
Copy link
Contributor Author

BKM14 commented Oct 28, 2024

Hey @Devessier, what do you think about this?

@Devessier
Copy link
Contributor

Hey @BKM14, thank you very much for your contribution. I made some changes to minimize the number of files that were changed. These files are often used across the application, and we can easily break something if we make a mistake.

@BKM14
Copy link
Contributor Author

BKM14 commented Oct 28, 2024

Yeah, I knew it was a little shabby given the number of files I had to change. I spent a whole day breaking my head trying to fix this.

Glad you minimized the changes.
Thank you.

</StyledControlLabel>
</StyledIconContainer>
) : null}
<StyledLabel>{selectedOption.label}</StyledLabel>
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to use OverflowingTextWithTooltipe here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! It works great. However, I'm not sure how accessible using this component here is.

`;

const StyledIconChevronDown = styled(IconChevronDown)<{
disabled?: boolean;
}>`
color: ${({ disabled, theme }) =>
disabled ? theme.font.color.extraLight : theme.font.color.tertiary};
flex-shrink: 0;
Copy link
Member

Choose a reason for hiding this comment

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

are all these flex-shrink mandatory? looks a bit too much everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I cleaned the layouting by using a unified grid layout.

`;

const StyledLabel = styled.span`
flex-grow: 1;
Copy link
Member

Choose a reason for hiding this comment

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

is this flex-grow necessary?
This code looks very similar to OverflowingTextWithTooltip

@Devessier
Copy link
Contributor

Thank you a lot for your time, @BKM14. I know truncating text can be tricky. The root issue was that the modal had no max-width, making it hard to truncate text, which requires setting a max-width beyond which the text should be truncated.

@charlesBochet charlesBochet merged commit ca54bc1 into twentyhq:main Oct 28, 2024
16 checks passed
Copy link

oss-gg bot commented Oct 28, 2024

Awarding BKM14: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/BKM14

@BKM14 BKM14 deleted the textOverflow branch October 28, 2024 15:24
charlesBochet pushed a commit that referenced this pull request Oct 31, 2024
Fixes:


![image](https://github.com/user-attachments/assets/900596ed-1426-49cd-a2f3-4b81eacbb7d0)

The regression is due a recent change I made to the SelectControl
component: #8125.

The SelectControls get applied `text-align: center` due to the styles
`react-datepicker` applies to the header component. My grid
implementation makes the label take all the available width. I could
have let it take an `auto` width, but I think it's better to set the
`text-align` property and ensure the `SelectControl` component behaves
predictably.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Country Select Picker Text Overflow Issue
3 participants