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

Add option for vertically stacked reading pane #2243

Merged
merged 9 commits into from Mar 26, 2021

Conversation

Phylu
Copy link
Contributor

@Phylu Phylu commented Dec 29, 2020

This resolves #116 by adding the option to enable a vertical reading pane.

@anwar3606
Copy link

we need this badly.., can someone please review this Mr?

@bengotow
Copy link
Collaborator

bengotow commented Jan 6, 2021

Wow thanks for picking this up @Phylu! Will look at this first thing tomorrow after I check out your other PR :-)

@Phylu
Copy link
Contributor Author

Phylu commented Jan 6, 2021

@bengotow Thanks for that. :)

In the meantime I also had the idea of it is possible to make the border between the mail list and the reading pane draggable to set the size of the different parts instead of having it 50/50 hardcoded. What do you think? If you have an idea on where to add that, please let me know.

@Phylu
Copy link
Contributor Author

Phylu commented Jan 12, 2021

@bengotow I also noticed, that the shortcuts (e.g. Del to delete an e-mail via the thread list) is not working anymore if the vertical stacked thread list is selected. Unfortunately, I was not able to figure out yet how to ensure that the events are correctly forwarded to the place where they are expected/needed.

@bengotow
Copy link
Collaborator

Hey @Phylu! Sorry for the delay reviewing this one - I'm pretty impressed you got this mostly working with so little code, wrapping the ThreadList and MessageList within a new component is a nice solution.

I think the problem you're seeing is that defining the splitVertical mode to have aThreadListVertical region / column instead of ThreadList also causes anything registered to WorkspaceStore.Location.ThreadList.Toolbar not to appear above the content. That's just the buttons in the top bar, but the way Mailspring works the keyboard shortcuts for an action is enabled whenever the button for the action is being rendered, so removing the buttons actually breaks the keyboard shortcuts too...

I think this actually really close though - instead of creating a new ThreadListVertical column in the new mode, you can scope down the existing ThreadList registration to just mode: ["split", "list"], and then register a new component (ThreadListVertical) for the ThreadList column in mode: ["splitVertical"].

Here's a branch where I made that change to test and it looks like it's working if you want to apply those changes here! 944eeeb. The only other thing in there (beside formatting changes) is that I explicitly rendered the MessageList.Toolbar location in the new vertical view so you get that row of buttons above the message.

I really like your idea of being able to drag a divider to make the sections change size - that's definitely the first thing people will ask for once they see this! 🙈

There's actually a component called "ResizableRegion" in the mailspring-component-kit that implements some drag-to-resize-a-div logic, but I'm not sure there's anywhere in the app where we use it vertically. The Sheet component uses it here if you want to copy this and give it a spin!

https://github.com/Foundry376/Mailspring/blob/pullrequests/Phylu/feature/vertical-reading-pane/app/src/sheet.tsx#L91:L91

…tical-reading-pane' into feature/vertical-reading-pane
@Phylu
Copy link
Contributor Author

Phylu commented Jan 19, 2021

Hi @bengotow,

thanks a lot for the help with that. I am going to check out your suggestion and update the PR accordingly. I will probably drop the resizing functionaliy for this PR and do it in another one to have the possibility to ship the feature as soon as possible as it was asked for quite often.

@Phylu
Copy link
Contributor Author

Phylu commented Jan 19, 2021

@bengotow I managed to get the resize functionality running. However, when no e-mail is selected, it does not work properly on my machine. I can see that the heigth attribute is changed correctly, but the computed style shown by the Dev Tools does not update. Do you have an idea what could be the issue?
I am also not sure if the import import { ResizableRegion, ResizableHandle } from '../../../src/components/resizable-region'; will make troubles in the future. Just let me know if there is anything for me to think of with regards to the code structure.

@Phylu
Copy link
Contributor Author

Phylu commented Feb 15, 2021

@bengotow [...] However, when no e-mail is selected, it does not work properly on my machine. [...]

This seems to be resolved with the electron update. I have therefore cleaned up the PR and it should be good to merge now.

@Phylu Phylu requested a review from bengotow February 24, 2021 17:06
@bengotow
Copy link
Collaborator

Hooray! Thanks @Phylu - I'll tweak this a tiny bit as we prepare for the next release and see if I can make a new image for the settings panel that illustrates the vertical split. Excited to get this shipped!

@bengotow bengotow merged commit c7f7dc0 into Foundry376:master Mar 26, 2021
@Phylu
Copy link
Contributor Author

Phylu commented Mar 26, 2021

Nice. @bengotow Feel free to do any additions that are needed for the next release. Let me know if there is anything else the I can contribute to this issue of needed.

@Phylu Phylu deleted the feature/vertical-reading-pane branch March 26, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Vertically stacked messages pane / reading pane ala Mail.app
3 participants