expose didExceedMaxLines from RenderParagraph #139962
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@text-exemption-reviewers" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
github-actions
bot
added
the
framework
|
Could you add a test to verify the method works? |
|
I added the test. But have a question to ask for some advice, i just want to expose a property, is applying for exemption better? |
Pull requests that add new code usually aren't eligible for test exemptions, as long as they introduce testable logic. In this case the test may not do much but it seems to be pretty straightforward to write. Thanks for adding one! |
I understand, thank you~ |
There was a problem hiding this comment.
Consider copying the comment from TextPainter.didExceedMaxLines?
There was a problem hiding this comment.
Maybe that's enough. Indicate its source and rough explanation.
Co-authored-by: Alex Li <[email protected]>
Co-authored-by: Alex Li <[email protected]>
huanghui1998hhh
force-pushed
the
expose_didExceedMaxLines
branch
from
7ef0bf6 to
9edfe61
Compare
| test('limited by ellipsis', () { | ||
| final RenderParagraph paragraph = createRenderParagraph(overflow: TextOverflow.ellipsis); | ||
| layout(paragraph, constraints: const BoxConstraints(maxWidth: 100.0)); | ||
| expect(paragraph.didExceedMaxLines, true); |
There was a problem hiding this comment.
Is this right? maxLines is null in this test, no?
There was a problem hiding this comment.
Due to this, TextPainter.didExceedMaxLines
If [maxLines] is null, this is true if [ellipsis] is not the empty string and there was a line that overflowed the
maxWidthargument passed to [layout]; otherwise it is false.
So that's right.
In previous commit, i add some comment for this. Then inspired by @AlexV525 , i think it's TextPainter.didExceedMaxLines's behavior, maybe doesn't need to be repeated here.
AlexV525
added
the
autosubmit
I want to build a widget that adds some extra functionality when the inner text overflow. So the problem occurred, I can't find an elegant way to determine if the text is overflowing. So i expose `didExceedMaxLines` from `RenderParagraph`, I think it can make sense. Have there some advice?
3 participants
I want to build a widget that adds some extra functionality when the inner text overflow. So the problem occurred, I can't find an elegant way to determine if the text is overflowing.
So i expose
didExceedMaxLinesfromRenderParagraph, I think it can make sense. Have there some advice?