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

[NOREF] Update TruncatedText component to take line clamp prop #1431

Open
wants to merge 12 commits into
base: feature/MINT-1732_echimp-ffs-cr-tdl
Choose a base branch
from

Conversation

garyjzhao
Copy link
Contributor

[NOREF]

Description

The TruncatedText component currently only takes a character limit. But recently, there have been designs asking to truncate text to x
amount of lines (line clamping) instead of character count. This PR updates the TruncatedText component to take in line clamp value

How to test this change

  1. Navigate to a CR and TDL side panel and see that CR Summary is line clamped to 5, instead of a character count.
  2. Look elsewhere in the app to ensure any other instances of TruncatedText still renders correct

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.
  • Updated the Postman Collection if necessary.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@garyjzhao garyjzhao requested a review from a team as a code owner October 17, 2024 17:44
@garyjzhao garyjzhao requested review from patrickseguraoddball and removed request for a team October 17, 2024 17:44
Comment on lines +8 to +20
const mocks = [
{
request: {
query: GetModelPlanBaseDocument,
variables: { id: 'f11eb129-2c80-4080-9440-439cbe1a286f' }
},
result: {
data: {
modelPlan: {}
}
}
}
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test throw error if Provider (and therefore mocks) were not included in test. Is there another way to solve this? Currently, this is just a random mock and not used in TruncatedText component

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the incorrect query to mock. TruncatedText utilized the MentionTextArea. We might actually might want to not use this. The MetionTextArea has a query embedded within it - useSearchOktaUsersLazyQuery. I forgot why I decided to utilize MentionTextArea. I don't think that was the right choice, considering how much overhead that component has built in. We should probably try to build TruncatedText without it. Sorry this was my fault

Copy link
Contributor

@patrickseguraoddball patrickseguraoddball left a comment

Choose a reason for hiding this comment

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

I dont think you addressed that unit test in your PR. It's mocking the incorrect query. Also we can replace MentionTextArea if you want, would be a good time. This would make us not need to mock the query

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.

2 participants