-
Notifications
You must be signed in to change notification settings - Fork 295
enh: redesign email display on phones #11754
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
enh: redesign email display on phones #11754
Conversation
Added a few changes such as the email content will now take the phone's screen full width, the topbar will partly hide itself on scroll, removed blank space under email, ... Signed-off-by: Tobiáš Vašťák <[email protected]>
Align skeleton loading to left for phones < 500px screen width Signed-off-by: Tobiáš Vašťák <[email protected]>
Make the phishing suspicion warning be aligned well with the rest of the content Signed-off-by: Tobiáš Vašťák <[email protected]>
|
What do you think @nimishavijay ? Any suggestions or ideas? |
|
Looks really nice! :) thanks for cleaning up the sticky header and the bottom spaces in addition. I see that right now the participants are sticky while I think it would make more sense of the title is sticky, don't you think? So I would suggest some small changes:
|
|
Okay, sure, makes sense to me :) So on scroll, it will display the first line of the subject and the parcipiants below, like it does when not scrolling. Then I have to fix the issue with unsubscribe button overlapping actions menu and that should be all |
|
And also... could this PR contain something like this? I think it looks better, what's your opinion on this @nimishavijay
|
8b0c5a7 to
470e39d
Compare
Make the email content full screen wide, when screen < 600 px. And a few other fixes Signed-off-by: Tobiáš Vašťák <[email protected]>
470e39d to
e1a3956
Compare
Fixes width of an event div, so it matches rest of the content. Signed-off-by: Tobiáš Vašťák <[email protected]>
|
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.) |
|
Update: I'll get back to this as soon as I can. Only the unsub button is remaing. I will post how it looks, hopefully, later today. |
Show only one line of the email subject. Signed-off-by: Tobiáš Vašťák <[email protected]>
|
I made a few changes and now it looks like: progress_update.mp4The animation can be a bit buggy, I'll try to fix that next time. |
|
Awesome! Looks really nice :) Regarding the unsubscribe button, it's definitely an improvement. It would be difficult to make sure that an email address of any length is shown fully if we want to keep the unsubscribe button on the same row, so I would say we should either
Since there are already many elements between the sender details and the body (phishing warning, show images warning, etc) I would vote for clicking on the sender to see the full email address (similar to what Gmail does in mobile). The unsubscribe fix can be done in a follow up btw! This is a great place to wrap up this PR design wise :) |
|
Okay, great! But I still have to make a few small fixes for the animation. After that, it should be ready. |
Make the animation of collapsing and expanding subject div less buggy and look a bit better. Signed-off-by: Tobiáš Vašťák <[email protected]>
|
Should be done! What do you think @ChristophWurst |
Signed-off-by: Tobias Vastak <[email protected]>
ChristophWurst
left a comment
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.
Tested, works really well
My nitpick is that I'm not a fan of query selectors and javascript styles. Where possible let's use pure CSS (with conditional classes and breakpoints). There is an isMobile mixin. You should find examples in other components. Use it to add classes specific to mobile.
| margin-inline-start: auto; | ||
| } | ||
| @media (max-width: 600px) { |
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.
I think there is a CSS var for the mobile breakpoint. Perhaps that would be a better option that the hard-coded 600px
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.
There is the --breakpoint-mobile: 1024px but 600-1024 px is wide enough to have an email displayed properly with the margin on the left. I'd keep it just for screens < 600px.
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.
Fair point. But for consistency I'd suggest we use the same breakpoint as in other places. There is no downside of the reduced left margin at 600-1024px.
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.
Okay so I'll fix this else in the next commit or in the diffrent pr.
| bottom: -2px; | ||
| background: linear-gradient(0deg, var(--color-main-background), var(--color-main-background) 90%, rgba(255, 255, 255, 0)); | ||
| @media (max-width: 600px) { |
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.
same
| }, | ||
| mounted() { | ||
| this.scrollEl = this.$el.closest('.app-content-wrapper') || window |
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.
nit: this gives me jquery vibes. is there any other way to achieve this?
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.
It's not jquery, it's native js.
$el is root element of the Vue component
.closest() looks for the closest parent
I'm not sure how else to do this, but I can take a look.
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.
Apologies that my message was confusing. I meant that the coding style reminded me of jQuery due to the element lookup by selector and direct DOM manipulation.
src/components/Thread.vue
Outdated
| }, | ||
| _measureHeightsOnce() { | ||
| const h2 = this.$el.querySelector('#mail-thread-header-fields h2') |
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.
use a vue ref
src/components/ThreadEnvelope.vue
Outdated
| .left { | ||
| display: flex; | ||
| align-items: center; | ||
| min-width: 0; /* umožní textu správně zkrátit */ |
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.
přeložit nebo odstranit komentář
Made a few changes to the code as suggested in pr (and to satisfy lint). Signed-off-by: Tobiáš Vašťák <[email protected]>
|
@ChristophWurst I made a few changes (like using ref instead of query selector) but some, like isMobile that you suggested isn't usable here, as we need to calculate weather to wrap the subject or not (it's not just phone or not). But still I have to make just a few small adjustements before merging - the main is when the subject is way too long (like 100 chars, and yes I received a subject this long for some reason) the animation keeps looping and the second one, I maybe, just maybe forgot to change 2 lines (in code) in the last commit back to what they should have been after testing something 😀 |
ChristophWurst
left a comment
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 responsive margin left works well. Let's get it in!
Let's leave out the header bar and its animation for now. I'm not fully convinced this is the best/only possible approach, and the animation is jittery right now.
Remove unnecesarry space above subject. Signed-off-by: Tobiáš Vašťák <[email protected]>
|
Okay, but a quick note - I think this concept in general is not bad. I tested it just now and I see it's a bit jiterry, definitely more than it was before. I think with a bit more work it could be the way. Just smooth it out (as it was before) and make it work for subjects > 100 chars. Making such fixes is definitely not hard and would be faster than remaking the whole thing, but if you don't like it, than sure, let's just push in only the responsive margin. EDIT: This is an edit I didn't expect you to react this fast and didn't want to put another comment 😅 |
|
This PR will be split into 2:
@ChristophWurst should I fix the animation and make a pr or leave it to someone else? |


Fixes #11663
Changes
Notes
Before
Mail.-.Nextcloud.-.Google.Chrome.2025-09-27.00-29-49.mp4
What I've done so far
Mail.-.Nextcloud.-.Google.Chrome.2025-09-27.00-31-36.mp4