Skip to content
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

🐛 FIX: message iframe vertically doesn't fit the container / available space #2222

Merged
merged 3 commits into from
Mar 31, 2020

Conversation

violoncelloCH
Copy link
Member

@violoncelloCH violoncelloCH commented Nov 7, 2019

Fixes #658

@jancborchardt
Copy link
Member

@GretaD was already working on this at #2218 :) Best coordinate on how to proceed

@violoncelloCH
Copy link
Member Author

so if we continue with this, there are still some things to do:
in order to remove the bottom (which preserves space for the attachments currently) and expand the iframe to the bottom of the viewport we still need to decide where to put the attachments instead. We could e.g. put an attachment indicator/button to the header which opens a popover or we could also have a floating indicator at the bottom which brings up a panel or something with the attachments. What do you think?

@jancborchardt
Copy link
Member

I think a floating indicator button for the attachments at the bottom is best, for the following reasons:

  • The header is already too crowded
  • Attachments are usually at the bottom
  • When it’s floating, they can easily be shown
  • It’s always a consistent size (just a button for "4 attachments" or in the case of 1 attachment it could show the info directly), and can expand on tap

@jancborchardt
Copy link
Member

@ChristophWurst does your 👀-Reaction on my comment mean that you approve? ;D

@violoncelloCH any other open questions on this, or help needed?

@jancborchardt jancborchardt added this to the 0.19.0 milestone Nov 21, 2019
@ChristophWurst ChristophWurst removed this from the 0.19.0 milestone Nov 21, 2019
@ChristophWurst
Copy link
Member

jancborchardt added this to the 0.19.0 milestone 38 minutes ago

Thou shall not mess with my release schedules :)

@ChristophWurst
Copy link
Member

I'm fine with that. But sounds like a lot more work. And I'm still not sold on setting an arbitrary height on the iframe size in the hope that it is big enough. We'll then also have a much larger scrolling pane, no? The attachements are fixed, yes, but what comes after the message contents ended?

@violoncelloCH
Copy link
Member Author

violoncelloCH commented Nov 21, 2019

@jancborchardt fine by me; it's time missing for me atm, not information... So if someone else wants to continue working on this I'd be fine... Maybe @GretaD ? Otherwise I'll try to find some time during the next weeks, but can't promise anything...sorry

@ChristophWurst this one explicitely doesn't set a fixed height but uses flex-grow which let's the iframe expand to the bottom of the viewport... Isn't this what we want to have?

@ChristophWurst
Copy link
Member

As far as I know you still can't have an iframe resize to the height of its contents dynamically, can you?

@violoncelloCH
Copy link
Member Author

just check out this PR and take a look how it's indeed possible 😉

@jancborchardt
Copy link
Member

jancborchardt commented Nov 22, 2019

As far as I know you still can't have an iframe resize to the height of its contents dynamically, can you?

This pull request increases the height of the iframe so the bottom edge lines up with the bottom edge of the viewport. It doesnt know about the contents of the iframe, but takes only the outside into account.

This results in what we want: Only a single scrollbar (albeit unfortunately hovering in the air).

So:

  • The height is not arbitrary
  • After the message contents, there's nothing. The attachments are floating anyway, since otherwise we have a tiny scroll and reading area

@jancborchardt
Copy link
Member

@violoncelloCH what’s left to do on this and how can I help? :)

@violoncelloCH
Copy link
Member Author

@jancborchardt well the basic css "magic" to make the iframe expand to the available screen height is there, however what's not yet implemented is the floating attachment button with a popover displaying the attachments (as you described in this comment #2222 (comment))... (Or we merge this without the attachment part? [so there would just be a little bit of polishing needed])
Sadly I'm currently too busy with work, some personal projects, etc, so I can't promise to find time for this soon :/

@jancborchardt jancborchardt added this to the 1.3.0 milestone Mar 25, 2020
@jancborchardt jancborchardt linked an issue Mar 26, 2020 that may be closed by this pull request
…e space

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
@jancborchardt jancborchardt force-pushed the fix/2110/html-iframe-vertically-fit-ccontainer branch from 3458042 to 1e41543 Compare March 26, 2020 18:56
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member

Fixed the 2 outstanding issues, this is ready for review! @nextcloud/mail

The only thing I was unable to fix is this which seems to need fixing in the components:

The button could also use some text next to the icon, namely here "2 attachments"

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good

@jancborchardt jancborchardt merged commit 78ff17c into master Mar 31, 2020
@jancborchardt jancborchardt deleted the fix/2110/html-iframe-vertically-fit-ccontainer branch March 31, 2020 17:59
@violoncelloCH
Copy link
Member Author

thanks very much @jancborchardt for taking this up and fixing the remaining issues...
I'm sorry I didn't find the time to come back to this earlier...

@jancborchardt
Copy link
Member

No worries @violoncelloCH! Thank you so much for your work! :)

@szaimen
Copy link
Contributor

szaimen commented Apr 2, 2020

@ChristophWurst is there already an ETA when a new release will be available? I would really like to test this out :)

@ChristophWurst
Copy link
Member

ChristophWurst commented Apr 2, 2020

No date set yet as we still work on stabilization, but feel free to test the betas https://help.nextcloud.com/t/call-for-testers-mail-v1-3/75825! I'll release another one soon so it will have this PR and other recent changes :)

@szaimen
Copy link
Contributor

szaimen commented Apr 2, 2020

Thanks, I will do that! :)

@rumbis
Copy link

rumbis commented Nov 3, 2021

.envelope{height:900px;}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML messages don’t extend to full height Email frame height too small after clicking show images
6 participants