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

Sidebar overflow fixes #257

Merged
merged 25 commits into from Mar 21, 2018
Merged

Sidebar overflow fixes #257

merged 25 commits into from Mar 21, 2018

Conversation

ghost
Copy link

@ghost ghost commented Mar 7, 2018

Description, Motivation and Context

There were some overflow problems if the window is too small:

  • the sidebar elements where squished if the window was smaller than the content height of the sidebar
  • there where multiple scrollbars if the window was smaller than the min-width of the sidebar and the width of the preview container

Fix:

  • added min-width to some elements
  • added an overflow-wrapper to the sidebar, that catches the sidebar overflow and displays a scrollbar if necessary
  • refactored the css a bit, for the coming resize feature, you now change the width of the sidebar with one css value here

Related Issue

#246

How Has This Been Tested?

Changes were tested on Ubuntu Gnome 16.04 by resizing the preview window from maximized down to 100px x 100px.

Screenshots (if appropriate):

These 3 scrollbars are the maximum, should you see more scrollbars in some case it's a bug:
bildschirmfoto-20180307200642-1916x1066

if the window is wide enough for the content there shouldn't be more than 2 scrollbars:
bildschirmfoto-20180307200850-2549x1069

no sidebar element should get squished anymore:
bildschirmfoto-20180307201034-2573x1340

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have included a migration scheme (If type of change is breaking change)

@ghost
Copy link
Author

ghost commented Mar 7, 2018

During testing i experienced some interesting behavior:
If you open the preview window the first time after app start it has a min-size, if you open it for the second time, it has not.

Info: the actions container isn't sticky yet, i thought we first check the overflow behavior with more sidebar content :)

@ghost ghost mentioned this pull request Mar 7, 2018
@hql287 hql287 added the bug label Mar 8, 2018
@hql287 hql287 added this to the Build 06 - v1.1.4 milestone Mar 8, 2018
@hql287 hql287 assigned ghost Mar 8, 2018
@hql287
Copy link
Owner

hql287 commented Mar 8, 2018

@jens-t: Working great so far 👍. Still has some (small) issues though:

  • I think the sidebar is a bit too wide at the moment (Yes, this is subjective).
  • When scroll, here's what it looks like on macOS:

screen shot 2018-03-08 at 10 50 54

Will try to make a fix for this.

@ghost
Copy link
Author

ghost commented Mar 8, 2018

I think the sidebar is a bit too wide at the moment (Yes, this is subjective).

Absolutely, it's just for showing how it looks, since it's simple to change that, i thought we discuss a good sidebar width here.

When scroll, here's what it looks like on macOS:

that's because of the min-content. i can also fix this, if you do, you should consider, that this version is the wanted behavior on win & linux. so, the extra space should only apply on macOS.

@hql287
Copy link
Owner

hql287 commented Mar 9, 2018

Absolutely, it's just for showing how it looks, since it's simple to change that, i thought we discuss a good sidebar width here.

@jens-t: I think the sidebar's width should be something between 200px and 215px. Eventually, I would like to make it resizable and this shouldn't be a problem anymore.

that's because of the min-content. i can also fix this, if you do, you should consider, that this version is the wanted behavior on win & linux. so, the extra space should only apply on macOS.

Yup, I noticed the difference of how electron render the titleBar on macOS vs Linux and Windows. This does make thing a little bit tricky though 🙈

Will get back to this after releasing the new update.

@hql287 hql287 removed this from the Build 06 - v1.1.4 milestone Mar 10, 2018
@ghost
Copy link
Author

ghost commented Mar 12, 2018

Will take a closer look at this tomorrow ...

@ghost
Copy link
Author

ghost commented Mar 13, 2018

added some functionality for the sidebar macOS window controls problem. there's a new wrapper element for that, so here is the new place to define the width of the sidebar. the 280px width of new SidebarWrapper matches the former 215px of the OverflowWrapper.

You can change the height of the top margin for the macOS window controls here. can't test if the 30px fit. theres probably more space needed.

i think this fix is the best way to handle the problem, what do you think? Maybe you can provide a screenshot of how this looks on macOS?

@ghost
Copy link
Author

ghost commented Mar 13, 2018

Do we still want to make the actionbuttons sticky? If yes, i would take a look at that, too.

@hql287
Copy link
Owner

hql287 commented Mar 14, 2018

If you open the preview window the first time after app start it has a min-size, if you open it for the second time, it has not.

Manta remembers the size & position of every window across sessions.
Unless you meant something else.

i think this fix is the best way to handle the problem, what do you think? Maybe you can provide a screenshot of how this looks on macOS?

Testing it now!

@hql287
Copy link
Owner

hql287 commented Mar 14, 2018

@jens-t: Looks like it's working. Nicely done 👍

On a second note, I'm thinking about making the title bar visible on macOS as well, so that it will look more consistent compared to Windows and Linux version.

screen shot 2018-03-14 at 12 42 37

Maybe this will be helpful for future UI customization as well. Any thought? 🧐

@hql287
Copy link
Owner

hql287 commented Mar 14, 2018

Update

You can change the height of the top margin for the macOS window controls here. can't test if the 30px fit. theres probably more space needed.

30px is a little tight. I tried to increase this but it somehow still doesn't feel very natural to me. I think there are some UI changes need to be made here to the sidebar. Right now we have a few issues which haven't been addressed such as:

  • The Accent Color component's colour picker is "floating" at the moment.
  • The Use custom color checkbox should belong to the toggle component.

Will play around with Sketch a bit and get back to you on this tonight!

@ghost
Copy link
Author

ghost commented Mar 14, 2018

Manta remembers the size & position of every window across sessions.
Unless you meant something else.

I meant that the first time you open the preview window you can just resize the window to a certain min-size (1024px x 800px, for example), when you open it the second time you can resize it to, for example 20px x 20px, so the second time there is no min-size for the window.

On a second note, I'm thinking about making the title bar visible on macOS as well, so that it will look more consistent compared to Windows and Linux version.

If thats the case, we wont need this fix, or?

Maybe this will be helpful for future UI customization as well. Any thought?

I think it's way less hacky, so yes i think it is a good idea to streamline the experiences, if it's an option for you.

The Accent Color component's colour picker is "floating" at the moment.
The Use custom color checkbox should belong to the toggle component.

I don't really understand whats the problem or the desired change, can you explain this in more detail or other words? 🙈 😆
So that i can make these changes ... annotated screenshots or sketchscribbles may help.

@hql287
Copy link
Owner

hql287 commented Mar 14, 2018

@jens-t : Sorry for not being clear.

I don't really understand whats the problem or the desired change, can you explain this in more detail or other words? 🙈 😆

What I meant with the accent color is that the color picker has "position: absolute" so even when we fix the overflow problem, this will still be an issue. That's why I think we should refactor it. Make sense? 😅

I think it's way less hacky, so yes i think it is a good idea to streamline the experiences, if it's an option for you.

Ok, let's make the title bar visible on macOS as well.

If thats the case, we wont need this fix, or?

We still do need this fix though, except the patch for macOS. I think it should be fixed with better design/UI. Just spent some time with Sketch but didn't like what I got, so I guess we should just proceed to fix the overflow issue for now.

I would like to spend more time to think about how we should handle the buttons.

screen shot 2018-03-15 at 00 06 38

screen shot 2018-03-15 at 00 06 44

screen shot 2018-03-15 at 00 05 16

@hql287
Copy link
Owner

hql287 commented Mar 16, 2018

On macOS, the bottom bar is usually called status bar. With this new status bar, we can even do things such as adding shortcuts to other actions in the future via icons:

icons

Or go dark 😎

darktheme

@hql287
Copy link
Owner

hql287 commented Mar 16, 2018

On improving the colour picker or creating a new one, I have a few suggestions:

  • It should have a colour wheel.
  • Has a default accent colour.
  • Add/Remove colours (with limitation such as minimum 1 and maximum 8 colours to be saved).

@ghost
Copy link
Author

ghost commented Mar 16, 2018

Unfortunately, yes 😄 I think it's has something to do with Manta rather than the component it self. Creating a new one is also a great option 👍. I like to have a color wheel there (like Sketch's colour picker) but it looks too clunky at the moment.

Ok, will get to that later in a seperate PR.

I think it could be helpful in the future when we have too many configs, but it shouldn't be the solution for the sticky buttons.

The grouping has nothing to do for me with the sticky buttons. Just asked this thinking about a redesign of the sidebar options.

Then I came up with this:
Let me know what you think.

I havn't understand yet, what problem the bottombar is solving. And there is also no bottom bar concept in windows or linux apps, so i think it kind of reminds me of the top bar macOS thingy from the beginning of the thread. but maybe thats part of a bigger question: Should the app-design relate to the macOS design or do we want to create a design that works across the different platforms in the same way, and has it's own look that also works and looks the same across platforms?

That doesn't mean that the bottom bar is a bad idea in all cases, maybe it's part of a concept i dont understand yet.

And does that mean that the actions in the main window are also moved to the bottom? To get the behavior more consistent across the app? Just thinking ...

On improving the colour picker or creating a new one, I have a few suggestions ...

Makes sense, i noted these points for later.

@hql287
Copy link
Owner

hql287 commented Mar 16, 2018

I havn't understand yet, what problem the bottombar is solving

Ahh, ok. Let's go back to the beginning and address the problems that we're dealing with. From the way I see it, we have the following issues:

  • The elements in the sidebar get squished when the user resizes the previewer window to a smaller size.
  • If the first problem is solved, then we will need to make sure the Save Configs and Export PDF visible or at least discoverable.

I think you've already solved the first issue. 👍

The second one is more tricky as we have to make it visible and also go with the flow of the sidebar when scrolling. The solution that I proposed earlier can solve this because:

  • It moves the two buttons to a different place where they're always visible.
  • The sidebar flow now will be much easier to deal with. Maybe a simple overflow-y: scroll would suffice?

And there is also no bottom bar concept in windows or linux apps

Maybe it's known by other names that I'm not familiar with but having used both Linux and Windows in the past I know that they do exist. For example:

On Windows

On Linux
screen shot 2018-03-16 at 20 47 00

But let's make things easier by NOT calling it status bar because it just happens to appear at the bottom of the window, where the status bar would appear, that's all. (The bar that I added doesn't even display any status).

So, let's call it a bottom bar as you suggested.

but maybe thats part of a bigger question: Should the app-design relate to the macOS design or do we want to create a design that works across the different platforms in the same way, and has it's own look that also works and looks the same across platforms?

Definitely the latter – a design that works across the different platforms in the same way, and has it's own look that also works and looks the same across platforms.

Btw, putting buttons at the bottom of a window is very common on many platforms, including Windows and Linux. I assume it's the gradient that makes it looks like something belongs to macOS? 🧐

And does that mean that the actions in the main window are also moved to the bottom? To get the behavior more consistent across the app?

Haven't thought about it yet. I tried putting the buttons of the previewWindow on top but it doesn't look right. That's why I put it on the bottom.

I'll try your suggestion and let you know how it goes.

@hql287
Copy link
Owner

hql287 commented Mar 16, 2018

Just noticed that @SkyzohKey is secretly watching this thread! 😄
Would love to hear your thoughts on this.

@ghost
Copy link
Author

ghost commented Mar 16, 2018

The second one is more tricky as we have to make it visible and also go with the flow of the sidebar when scrolling ... It moves the two buttons to a different place where they're always visible.

I think that's also solved in the current version of the PR with a later commit, that's why i wondered about the bottom bar. Does the current version not work for you, is it buggy on macOS? For me it is working great. Design is something that can be changed, i mean the sticky behavior itself.

I also thought the design doesn't have to be perfect for now, cause we're discussing a greater redesign ... at least it fits the current design state of the app :)

Definitely the latter – a design that works across the different platforms in the same way, and has it's own look that also works and looks the same across platforms.

Ok, good to know 👍

Btw, putting buttons at the bottom of a window is very common on many platforms, including Windows and Linux. I assume it's the gradient that makes it looks like something belongs to macOS?

Maybe you're right 😄 And maybe also the blue/white button combo ... i thought a bit more about the button bar, and maybe we should do some tests with it :)

Some things, that are not that ideal for me:

  • is that the bottom bar with 100% window width also makes the viewport for the preview area smaller, thats not the case when it is part of the sidebar. Maybe that's not so bad ...
  • or that it's a long way from a setting in a sidebar to the export button ... maybe also not a big deal

But the extra space for new prominent elements could be a good idea for the mainWindow, don't know what to place there in the previewWindow, i think in the preview window belongs only preview stuff. Or are there new preview features coming that need that space?

The other good thing is that you can display the buttons next to each other, but the question is, is it worth the disadvantages.

I'm not sure yet, what's my final opinion on that 😄

Haven't thought about it yet. I tried putting the buttons of the previewWindow on top but it doesn't look right. That's why I put it on the bottom.

Yeah, if it's 100% window with, i also think at the bottom is better.

@hql287
Copy link
Owner

hql287 commented Mar 17, 2018

And maybe also the blue/white button combo

That's true! 😄

or that it's a long way from a setting in a sidebar to the export button

I really don't think this is an issue.

In my opinion, this may not be the perfect solution but it does solve all of our issues with one small disadvantage that you mentioned, which is smaller viewport for previewing the whole invoice. For this, I think we can we can introduce a new feature to allow the user preview the invoice in fullscreen mode. Maybe something like this:

fullscreen

Anyway, if you still want to explore other solutions, feel free to take the time to work on it. We can keep this PR open until then.

Or we can choose this as a "good enough" solution at the moment to finish this PR and move on. Then maybe get back to it at some other time in the future.

Let me know what you think.

@ghost
Copy link
Author

ghost commented Mar 17, 2018

or that it's a long way from a setting in a sidebar to the export button

I really don't think this is an issue.

Ok :)

we can introduce a new feature to allow the user preview the invoice in fullscreen mode

Thats a good idea!

Or we can choose this as a "good enough" solution at the moment to finish this PR and move on. Then maybe get back to it at some other time in the future.

Yeah, this! Let's get the fix out first and let's work on a more prefect solution later :)

Anyway, if you still want to explore other solutions, feel free to take the time to work on it.

Let's work on that in a seperate PR. Maybe this could also be part of a greater redesign PR ...

Some questions about the future PR / Bottom Bar:

  • What are the functions of the other bottom bar icons?
  • Should both, the mainWindow bottom-bar and the previewWindow bottom bar include all these icons/actions?
  • if yes, does the cog for example, open a seperate settingsWindow or does it link to the settings view of the mainWindow?

i would display the previewWindow bottom-bar only with the action buttons and the fullscreen icon.
And the mainWindow bottom bar with all the other icons / actions ... they probably stand for settings, user data sync, new invoice? and some kind of hide somthing? :)

And: I think the bottom-bar has the potential to help with a more intuitive user flow, if we get it right. Reaching prominent settings with one click is always a good thing.

@hql287
Copy link
Owner

hql287 commented Mar 17, 2018

What are the functions of the other bottom bar icons?

This's just a mockup at the moment so I don't know what actions those icons will perform, yet. They're like placeholders and we don't even need to include them in this PR. (Unless you already have something in mind)

Should both, the mainWindow bottom-bar and the previewWindow bottom bar include all these icons/actions?

No, I don't think so. Since mainWindow and previewWindow are 2 separate windows with 2 separate responsibilities, I don't see any reason to include these icons in both windows.

We might do this if both windows have shared action(s) though.

i would display the previewWindow bottom-bar only with the action buttons and the fullscreen icon. And the mainWindow bottom bar with all the other icons / actions ... they probably stand for settings, user data sync, new invoice? and some kind of hide somthing? :)

Yeah, this is where we can get creative! 😁

And: I think the bottom-bar has the potential to help with a more intuitive user flow, if we get it right. Reaching prominent settings with one click is always a good thing.

👍

Yeah, this! Let's get the fix out first and let's work on a more prefect solution later :)

So would you like to finish this PR with this proposed solution?

@ghost
Copy link
Author

ghost commented Mar 17, 2018

This's just a mockup at the moment so I don't know what actions those icons will perform, yet. They're like placeholders and we don't even need to include them in this PR. (Unless you already have something in mind)

Ah ok, understand ... thought you had already something in mind for them.

No, I don't think so. Since mainWindow and previewWindow are 2 separate windows with 2 separate responsibilities, I don't see any reason to include these icons in both windows.
We might do this if both windows have shared action(s) though.

Yeah, that's what i would have suggested too.

Yeah, this is where we can get creative!

😄

So would you like to finish this PR with this proposed solution?

It already is, or am i missing something?

@hql287
Copy link
Owner

hql287 commented Mar 17, 2018

It already is, or am i missing something?

@jens-t: Have you implemented the bottom bar yet?

@ghost
Copy link
Author

ghost commented Mar 17, 2018

@jens-t: Have you implemented the bottom bar yet?

I think we've talked past each other 😆 i thought we talked about adding the bottom bar in a later PR ... cause adding it, means big changes in the html of the mainWindow and previewWindow, needs more design thinking, and so on ...

Maybe because we talked about "the fix", for me,"the fix" is already there, all Problems the bottom-bar would solve in another way are aleady solved in the current state. Thats why i asked if you already took a look at the current state of the PR ...

@hql287
Copy link
Owner

hql287 commented Mar 18, 2018

@jens-t: Ahh, haha ok! 😄 Sorry for not making it clear. Let's do it in another PR then. 👍

(FYI, I did take a look at the current state of this PR, it's working but I thought you said it felt hacky or tricky somewhere so I thought you're not happy with it as well. That's why I proposed the bottom bar solution. Guess the conversation was a bit long so both of us got confused.😄)

Anyway, I looked at commits again and noticed that we can make it works by simply removing the flexbox effect from the sidebar completely. So it'll be just a normal div with overflow: scroll enabled. As for the sticky buttons, just make it fixed at the bottom and we're good to go.

I just refactored the code a little bit and it works exactly the same way. Please let me know if it works on Linux as well. If there's no potential side-effect then I think we can move on :)

@ghost
Copy link
Author

ghost commented Mar 18, 2018

(FYI, I did take a look at the current state of this PR, it's working but I thought you said it felt hacky or tricky somewhere so I thought you're not happy with it as well. That's why I proposed the bottom bar solution. Guess the conversation was a bit long so both of us got confused.smile)

I only said, it was tricky to get it right. With that i meant it was hard. But it was working perfectly and was a valid & not hacky solution. But now i understand the confusion.

Anyway, I looked at commits again and noticed that we can make it works by simply removing the flexbox effect from the sidebar completely. So it'll be just a normal div with overflow: scroll enabled. As for the sticky buttons, just make it fixed at the bottom and we're good to go.

I tried your solution too, but that doesn't work for linux & windows, because the scrollbar is hidden behind the actions section:
bildschirmfoto-20180318125347-516x614
The scrollbars on macOS are working differently, so you couldn't see this bug. The strange thing is, that the scrollbars on linux in "not electron" apps are also working differently. Chromium uses it's own custom scrollbars on linux, not the GTK ones, thats why they are different in electron apps too. Maybe that wouldn't be so bad, if they wouldn't look that oldschool 😆

So, i think we should revert the last 3 commits and then we're good to go :)

@ghost
Copy link
Author

ghost commented Mar 18, 2018

FYI: this problem also occurs in the mainWindow, saw this some time ago and thought, we can fix that later:
bildschirmfoto-20180318131144-626x287

@hql287
Copy link
Owner

hql287 commented Mar 21, 2018

@jens-t: It's merged! Sorry for the delay, something came up and required my immediate attention in the last couple of days.

@ghost
Copy link
Author

ghost commented Mar 21, 2018

@jens-t: It's merged! Sorry for the delay, something came up and required my immediate attention in the last couple of days.

No problem! I also got ill, and am still recovering. Will get back on the Template refactoring in a couple of days ✌️

@hql287
Copy link
Owner

hql287 commented Mar 21, 2018

@jens-t: Speedy recovery bro! 👍

@ghost
Copy link
Author

ghost commented Apr 25, 2018

Thanks, i'm finally healthy again, but have some other projects at the moment that need my focus. I get back to you and manta in the near future ... just fyi :)

@hql287
Copy link
Owner

hql287 commented Apr 29, 2018

@jens-t: Ahh, it's good to have you back, bro 👍 No pressure, any help is greatly appreciated as always! :)

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

Successfully merging this pull request may close these issues.

2 participants