-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
do not cut off bottom portion of text #2568
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
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7211041
to
81a470e
Compare
81a470e
to
87ed516
Compare
Thanks for your contribution. But I think this is by design, and you can modify it through the theme properties. :root {
--heading-line-height: normal;
} |
I'd tend to include this fix, as by default I would expect line-height to be automatic by default in all Browsers and not require additional changes. |
87ed516
to
9ddb2e6
Compare
@sy-records ah, thanks; I did not check for an existing variable. I set the global variable and it did indeed fix the problem but as @paulhibbitts commented, I would expect the text to look correct by default in all browsers. I've moved the change to be the default |
Thanks @dajrivera , now that I think about it I may also have run across this issue earlier when trying different fonts out with v5 and noticing I needed to then further adjust line-height to avoid slight cut-off one some particular ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* calculation must use units; 'normal' value breaks calculation * replace --header-line-height variable with line height unit (lh) * introduce line heigh multiplier to reduce or increase line height
@sy-records looks like the h2 bottom-margin is calculated and was the reason for the prior "Unit required" comment; using value normal here broke that calculation. I've updated the bottom-margin calculation using the line height unit instead. At this point the bottom margin was too large so I included a local line-height-multiplier to reduce the margin size as a workaround. Let me know if you can think of a better alternative or a more appropriate method to implement this. Looks good to me in Edge. I can test Chrome, Firefox, and Safari tomorrow. |
Good catch @sy-records , my apologies for not viewing additional pages in the PR preview... I will be more careful in the future. From my perspective I hope that the resulting spacing unit adjustments match the spacing as originally specified by John Hildenbiddle as much as possible. |
* change h2 margin calculation to use lh and em units to allow heading-line-height var to be any valid value (no unit requirement)
e3a16b8
to
b814175
Compare
@sy-records @paulhibbitts Please review this again. I've changed the approach to only allow y overflow on h1 & h2 headings. Tested this on Edge, Chrome, Firefox, and Safari and all display the fonts without issue without additional changes to the rest of the UI. If OK, let me know I can clean up the commits. |
Thanks @dajrivera , but it looks like it strays quite a bit from the original theme spacing, re: H2 border and spacing. |
b814175
to
fdce5fe
Compare
@paulhibbitts sorry about that, seems the calc function went missing when reverting a change that was affecting safari; I've re-added it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a text clipping issue where the bottom portion of text gets cut off in Edge, Chrome, and Firefox browsers. The fix involves modifying the overflow CSS properties to prevent vertical text clipping while maintaining horizontal overflow control.
- Replaces single
overflow: hidden
property with separateoverflow-x
andoverflow-y
properties - Sets
overflow-x: clip
to handle horizontal overflow andoverflow-y: visible
to prevent vertical text clipping
@@ -251,7 +251,8 @@ | |||
|
|||
/* Prevent long titles from causing horizontal scrolling */ | |||
&[id] a { | |||
overflow: hidden; | |||
overflow-x: clip; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overflow-x: clip
property has limited browser support compared to overflow-x: hidden
. Consider using overflow-x: hidden
for better browser compatibility, as clip
may not be supported in older browsers.
overflow-x: clip; | |
overflow-x: hidden; |
Copilot uses AI. Check for mistakes.
Summary
The bottom portion of texts get cut off on Edge, Chrome, & Firefox; this issue does not appear in Safari. Using CSS property
line-height: normal
seems to fix the issue and does not affect Safari's rendering of the page.before:

after:

Related issue, if any:
N/A
What kind of change does this PR introduce?
Other: minor UI fix
For any code change,
Does this PR introduce a breaking change?
No
Tested in the following browsers: