Skip to content

fix(Truncate): fix text duplication in middle position #11417

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

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

mfrances17
Copy link
Contributor

What: Closes #10858

Additional issues:

@mfrances17 mfrances17 requested a review from a team January 14, 2025 17:11
@mfrances17 mfrances17 self-assigned this Jan 14, 2025
@mfrances17 mfrances17 requested review from tlabaj and kmcfaul and removed request for a team January 14, 2025 17:11
@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 14, 2025

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

The portion that renders the content for truncated middle may also need to be updated as well. Right now what gets rendered (using the content from the issue) is the first and second character of the content string:

image

Since we're just returning the str param in the sliceContent helper instead of an array.

@mfrances17 mfrances17 force-pushed the truncate-mid-rep-text branch from 06a7b4c to b562114 Compare January 22, 2025 15:21
@mfrances17
Copy link
Contributor Author

after some investigation, the real issue with the original logic is that a negative value could be passed to slice, which acts as an offset to the original content string and results in a positive length that the original logic does not assume correctly. it should work now as expected.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks much better 👨🏼‍🍳 🤌 Would be worth adding another middle truncate test when the content length is less than the trailingNumChars prop (currently we only have 2 that are both cases where content is longer than the prop value).

@mfrances17 mfrances17 force-pushed the truncate-mid-rep-text branch from 642ce1c to 4ad7168 Compare January 24, 2025 23:01
@mfrances17
Copy link
Contributor Author

@thatblindgeye i added a bonus middle truncate test in which the length of the trailing chars exceeds the length of the original string

@thatblindgeye thatblindgeye merged commit 711a561 into patternfly:main Jan 24, 2025
13 checks passed
mattnolting pushed a commit to mattnolting/patternfly-react that referenced this pull request Feb 14, 2025
)

* fix(Truncate): fix text duplication in middle position

* fix issue with negative slice value

* layout issues caught by tests

* add middle truncate test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - [Truncate] - middle truncate renders wrong in some cases
4 participants