Remove overflow: hidden from border-radius utility classes#710
Remove overflow: hidden from border-radius utility classes#710selamw1 wants to merge 1 commit intogoogle:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the overflow: hidden property from the border-radius utility classes to fix a text clipping issue. The change is logical, but it lacks corresponding tests. As per the repository's style guide, all code changes should be tested. I've added a comment to highlight the need for a test case to prevent future regressions.
|
|
||
| .border-ow-${idx} { outline-width: ${idx}px; } | ||
| .border-br-${idx} { border-radius: ${idx * grid}px; overflow: hidden;}`; | ||
| .border-br-${idx} { border-radius: ${idx * grid}px; }`; |
There was a problem hiding this comment.
Per the repository style guide, code changes should include tests. To prevent this text clipping issue from recurring, please consider adding a test case (e.g., a visual regression test) that verifies this fix.
References
- The repository style guide states: 'If there are code changes, code should have tests.' (link)
selamw1
requested review from
dmandar,
gspencergoog and
jacobsimionato
as code owners
|
I think you'll need to revert your changes to the package-lock file to get the checks to pass. |
selamw1
force-pushed
the
fix/lit-renderer-text-clipping
branch
from
830f700 to
011f5eb
Compare
|
Done, reverted the lock file and squashed all changes into one commit. |
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Great, thanks for cleaning that up!
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Actually, looks like there's one merge conflict to resolve, and then I think this is good to merge.
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Actually, looks like there's one merge conflict to resolve, and then I think this is good to merge.
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Actually, looks like there's one merge conflict to resolve, and then I think this is good to merge.
|
Thanks, conflict resolved. |
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Oh weird GitHub sent my request for changes multiple times, sorry about that!
Anyway, it looks like there's a check failing, it might be a build order-of-operations issue from first glance.
selamw1
force-pushed
the
fix/lit-renderer-text-clipping
branch
from
cd584f5 to
d07bff1
Compare
|
CI failure fixed. |
selamw1
force-pushed
the
fix/lit-renderer-text-clipping
branch
from
d07bff1 to
3019ecf
Compare
|
Adjusted the test script to use find and xargs to fix the globbing issues in the CI runner. Tests are discovering and passing locally now. |
2 participants
It fixes issue #208
This PR resolves an issue where text elements using the Lit renderer were unexpectedly clipped when rendered with rounded corners.
The border-br-* utility classes in @a2ui/web_core (which handle border-radius) were indiscriminately applying overflow: hidden. This caused clipping for content like line-height ascenders/descenders in text blocks.
This commit removes the overflow: hidden rule from the border-radius class generator, fixing the clipping while maintaining the intended rounded corners.