SD-1474: bug - some horizontal lines slightly overlap text#1841
SD-1474: bug - some horizontal lines slightly overlap text#1841johanneswilm wants to merge 7 commits intosuperdoc-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1061dc4c63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@johanneswilm nice fix — using negative offsets to push borders into margins makes sense.
one thing to watch: paragraphs with both shading and space > 0 will show a gap between the fill and the border. fine as a follow-up.
small DX suggestion on the repeated offset math. test coverage is good — consider adding a visual regression .docx too.
left inline comments.
…bug-some-horizontal-lines-slightly-overlap-text
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@johanneswilm shading follow-up is split off to SD-2106, border loop discussion is settled — all good.
code is correct, approving. feel free to merge after the inline comments are addressed, if relevant.
…bug-some-horizontal-lines-slightly-overlap-text
…bug-some-horizontal-lines-slightly-overlap-text
|
Hey @caio-pizzol I've made the changes you requested. Unfortunately I don't have any access rights in relation to merging. That's fine with me, but it means someone else has to merge. What would be more important to me would be to get the access rights needed so that I can add tags to PRs. Otherwise it's a bit difficult to communicate that a specific branch should not be merged, etc. . I'm doing that by putting it in the PR description, but it's easily overlooked. |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @johanneswilm, thanks for the updates — the tests and comment cleanup look good.
before merging, I spotted one more thing: space is in points but width is in pixels, so adding them together in the offset calc is slightly off. left an inline comment with the fix.
re: access — I'll get you tag permissions on the repo so you can label PRs. makes sense.
| if (!attrs?.borders && !attrs?.shading) return {}; | ||
| const borderBox = getParagraphBorderBox(fragmentWidth, attrs.indent); | ||
|
|
||
| // Calculate border positioning to extend into margins |
There was a problem hiding this comment.
space is in points but width is in pixels — need to multiply space by PX_PER_PT before adding. same fix on the other three sides, and test expected values will need updating.
|
closing in favor of #2324. thanks for the work on this. |
No description provided.