fix: restore sender copy-to-clipboard and fix thread subject alignment#12966
fix: restore sender copy-to-clipboard and fix thread subject alignment#12966sturlan wants to merge 3 commits into
Conversation
|
Thanks for opening your first pull request in this repository! ✌️ |
jancborchardt
left a comment
There was a problem hiding this comment.
According to the screenshots, the alignment is not fully fixed:
- The people chips of "To" and "Cc" should have same indentation as those of "From"
- The horizontal whitespace between the chips should be the same as the vertical whitespace between the chips
- There's very little padding on the right of the inside of the chips
@nimishavijay for review as well
|
/backport to stable5.8 |
|
Should i wait @nimishavijay before proceeding? |
|
Nice! Agree with the feedback @jancborchardt gave, plus I'd also add that the chips itself are quite large and blocky. I was think it could be smaller like the NcUserbubble component. Do you think we could get the styling to look similar? |
|
Hi @nimishavijay thanks for the feedback! I've been trying to get the chip styling right but I'm struggling with it — would you be open to taking ownership of the CSS for the recipient display? I'd be happy to leave that part to you and focus on the functional side of things. The current state uses NcUserBubble directly with no custom CSS overrides, so it should be a clean slate for you to work from. |
Of course, I can definitely try my hand. would be better off as a follow up (opened it here #12989) so you are not blocked :) |
That's great, in that case, if @ChristophWurst and @jancborchardt agree to can close this PR as i addressed the fixes and issues in two commits, one for fixing and the other to remove all custom css so you can have a clean start |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
- Replace <p class="sender__email"> with RecipientBubble when expanded and hasRecipients is false, so clicking the sender email always opens the contact popover (reply, add to contact, copy to clipboard) - Add a From: row with RecipientBubble at the top of the expanded recipients section, restoring copy-to-clipboard for the sender when To/Cc/Bcc recipients are present (fixes nextcloud#12952) - Fix #mail-thread-header-fields padding-inline-start from the hardcoded 66px to calc(var(--default-grid-baseline)*14 + var(--border-radius-container) + 2px) to account for the --border-radius-container header padding introduced in PR nextcloud#12666 (fixes nextcloud#12946) Follows up on PR nextcloud#12666. AI-assisted: Claude Code (claude-sonnet-4-6) Signed-off-by: Darko Sturlan <darko.sturlan@gmail.com>
f60798d to
9ff6996
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
✅ Action performedReview finished.
|
|
@sturlan please post up to date before/after screenshots. Thanks! |
📝 WalkthroughWalkthroughThe thread header padding was recalculated from design variables, and the expanded envelope view now shows sender information as recipient bubbles, including a new ChangesThread header and sender display
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab6784ff-e450-44d1-9602-18cda2b48a8e
📒 Files selected for processing (2)
src/components/Thread.vuesrc/components/ThreadEnvelope.vue
| // 66px to allign with the sender Envelope -> 8px margin + 2px border+ avatar -> 40px width + envelope__header -> 8px padding + sender-> margin 8px | ||
| padding-inline-start: 66px; | ||
| // envelope margin (2×baseline) + border (2px) + header padding (--border-radius-container) + avatar (10×baseline) + sender margin (2×baseline) | ||
| padding-inline-start: calc(var(--default-grid-baseline) * 14 + var(--border-radius-container) + 2px); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace hardcoded 2px with a design token variable.
+ 2px introduces a magic dimension in SCSS. Use a Nextcloud CSS variable (or derive from one) to keep spacing/dimensions tokenized.
Suggested change
- padding-inline-start: calc(var(--default-grid-baseline) * 14 + var(--border-radius-container) + 2px);
+ padding-inline-start: calc(var(--default-grid-baseline) * 14 + var(--border-radius-container) + (var(--border-width-input-focused) * 2));As per coding guidelines, "Use Nextcloud CSS variables for all CSS colors, spacing, and dimensions; do not use magic numbers."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| padding-inline-start: calc(var(--default-grid-baseline) * 14 + var(--border-radius-container) + 2px); | |
| padding-inline-start: calc(var(--default-grid-baseline) * 14 + var(--border-radius-container) + (var(--border-width-input-focused) * 2)); |
Source: Coding guidelines
Hi Christoph, |
|
@ChristophWurst can this be merged now? |
|
Waiting for @jancborchardt on the design review part |
|
My review comments seem to still be happening on the screenshots you posted @sturlan? Let me know if you need me to be more specific. :) |
|
Would be worth updating the branch to latest main to get the changes from #13118 in. |
|
Hi @jancborchardt — those three styling points are being handled in the follow-up #12989 by @nimishavijay. In the second commit (f60798d) I removed all custom CSS from the recipient section to give her a clean slate. Would you be able to mark your review as resolved / approved so this can move forward? @ChristophWurst will do — rebasing onto latest main now to pick up #13118. |
|
@sturlan please post up to date before/after screenshots. Thanks! |
Hi, I only have after sceenshots. Will that be a problem? |
|
These screenshots are outdated. They do not represent the latest state. The design fixes of the other branch are not reflected. |
|
Oh, yes i see what you mean, i will pull the latest main and take sceenshots again |
|
You already pulled and merged. Only the screenshots are missing. |



Summary
Follow-up to #12666, fixing two regressions it introduced.
#12952 — Lost ability to copy sender email to clipboard
Before #12666 the sender email was rendered as a
RecipientBubble(contact popover with Reply / Add to Contact / Copy to clipboard). #12666 replaced it with a plain<button>or<p>— no popover in either case.Fix:
<p class="sender__email">withRecipientBubblefor the no-recipients case (sender email always clickable when expanded)RecipientBubbleat the top of the expanded recipients section, restoring the contact popover when To/Cc/Bcc are present#12946 — Regressed thread subject alignment
#mail-thread-header-fieldshadpadding-inline-start: 66px, computed assumingenvelope__headerhad 8 px of left padding. #12666 changed that padding from--border-radius-element(8 px) to--border-radius-container(12 px),leaving the sticky header 4 px short.
Fix:
Test plan
Fixes #12946
Fixes #12952
Summary by CodeRabbit
New Features
Style