-
Notifications
You must be signed in to change notification settings - Fork 667
fix(ui): ensure text wrap works in HunkDiff #9120
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@nshcr is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
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 enables text wrapping in the HunkDiff component by replacing the fixed white-space: pre
with a dynamic pre-wrap
when wrapText
is enabled.
- Updated the inline
style
on the<td>
to set a--pre-wrap
CSS variable based onwrapText
. - Changed two hardcoded
white-space: pre
declarations towhite-space: var(--pre-wrap)
.
Comments suppressed due to low confidence (2)
packages/ui/src/lib/hunkDiff/HunkDiffRow.svelte:213
- [nitpick] The CSS variable name
--pre-wrap
could be confusing since it doesn’t clearly indicate it maps to thewhite-space
property. Consider renaming it to something like--whitespace-value
or--white-space-setting
.
style="--tab-size: {tabSize}; --wrap: {wrapText ? 'wrap' : 'nowrap'}; --pre-wrap: {wrapText
packages/ui/src/lib/hunkDiff/HunkDiffRow.svelte:298
- We haven’t added any unit or visual tests to verify that
wrapText
actually causes wrapping. Consider adding a Jest/Playwright test or snapshot to cover this new behavior.
white-space: var(--pre-wrap);
@@ -303,7 +305,7 @@ | |||
position: relative; | |||
min-height: 18px; | |||
text-wrap: var(--wrap); | |||
white-space: pre; | |||
white-space: var(--pre-wrap); |
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 white-space: var(--pre-wrap)
declaration is duplicated in two selectors. To improve maintainability, consider extracting it into a shared class or mixin to avoid repetition.
Copilot uses AI. Check for mistakes.
Thanks a lot for contributing! Do you have before-after screenshots by any chance? Besides that, someone with better Csskills will take a look soon :)! |
Kapture.2025-06-22.at.14.38.43.webm@Byron Of course! I recorded a video showing the difference before and after applying this commit. The dev build was run based on the latest commit on master. By the way, this issue likely exists in the v2 version of the component as well. Since I noticed that it has already been deprecated, I didn't make any changes there. |
I just resolved the merge conflicts, and still waiting for review 🙂. |
Thanks so much! This PR has been in the queue for so long and was a fix for a real issue. And it appears I somehow dropped the ball on this, I am so sorry for that! Let's have @PavelLaptev take a last look before merging. |
@Byron No worries! |
it didn't work
"Soft wrap" more clearly indicates that this is visual wrapping (lines appear wrapped on screen) vs "hard wrap" (actually inserting line breaks), which is particularly important in a diff context where the actual content shouldn't be modified. Since this is specifically for diff/code viewing, "soft wrap" aligns better with developer expectations and terminology.
@nshcr thanks for the PR! I made a few additions:
Will merge it in a few hours. |
@PavelLaptev Thanks for your additional commit! |
All good — that's a nice fix, thank you! |
The Appearance -> Text wrap option in Global Settings doesn't seem to have any effect.
I found that the issue was caused by the
white-space: pre
style, which was preventing text from wrapping.So I changed
white-space
topre-wrap
whenwrapText
is true. It should be working correctly now.Ref: https://developer.mozilla.org/en-US/docs/Web/CSS/white-space