Patch #30435

Replace float rules with flexbox for content and sidebar block

Added by Anonymous 10 months ago. Updated 2 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:UI
Target version:4.1.0

Description

What this patch does:
1. It moves the sidebar to be rendered after the content block in base.html.erb, since natural flow of HTML should be from left to right, so no point in rendering sidebar before the content block, because it doesn't match our base layout.

2. It sets the #main block to a flexbox and removes a redundant background color definition rule from it.

3. It changes the % based width of the sidebar to a fixed 220px (if you think it should be wider, feedback is welcome), because on wide screens 22% is quite a lot of space!

4. #content block is the only one that will stretch now.

main-block-fixes.diff Magnifier (2.46 KB) Anonymous, 2019-01-15 12:41

main-block-fixes-no-swap.diff Magnifier (1.57 KB) Anonymous, 2019-01-17 07:45

sidebar.png (289 KB) Marius BALTEANU, 2019-01-17 21:28

main-block-fixes-no-swap-min-max.diff Magnifier (1.67 KB) Anonymous, 2019-02-09 00:34

main-block-fixes-no-swap-min-max_no_shrink.diff Magnifier - Max J.'s patch with flexbox constraints (1.68 KB) Bernhard Rohloff, 2019-02-14 18:28

30435-screenshot-20190225.png (29.9 KB) Go MAEDA, 2019-02-25 07:23

current.png (421 KB) Marius BALTEANU, 2019-03-27 23:46

after.png (455 KB) Marius BALTEANU, 2019-03-27 23:46

flex_for_content_and_sidebar.patch Magnifier (1.8 KB) Marius BALTEANU, 2019-03-27 23:50

table-bug@2x.png (79.7 KB) Go MAEDA, 2019-04-28 12:46

responsive-multiresolution-sidebar.patch Magnifier (1.12 KB) Antonio McDeal, 2019-09-04 19:23


Related issues

Related to Redmine - Patch #32037: Constrain sidebar width on different resolutions Closed

Associated revisions

Revision 18033
Added by Go MAEDA 8 months ago

Replace float rules with flexbox for content and sidebar block (#30435).

Patch by Max Johansson and Marius BALTEANU.

Revision 18135
Added by Go MAEDA 6 months ago

Fix layout problem when showing wide tables (#30435).

Patch by Marius BALTEANU.

History

#1 Updated by Marius BALTEANU 10 months ago

  • Subject changed from Change #main block to a flexbox + additional minor fixer to it's direct children. to Change #main block to a flexbox + additional minor fixes to it's direct children.

#2 Updated by Go MAEDA 10 months ago

  • Target version set to 4.1.0

It looks nice. Setting the target version to 4.1.0.

#3 Updated by Marius BALTEANU 10 months ago

Please do not commit because I want to test more these changes. I looked yesterday for few seconds and I saw some problems.

#4 Updated by Marius BALTEANU 10 months ago

  • Assignee set to Marius BALTEANU

#5 Updated by Bernhard Rohloff 10 months ago

The patch looks good to me, although I'm not sure if the width is sufficient for the text length of everbody's saved queries. It also wraps the heading of "Custom queries" if you choose German ( it's "Benutzerdefinierte Abfragen" ) as interface language. No big deal to me but I think it's worth mentioning.

Overall, I think the patch is a great improvement.

+1

#6 Updated by Anonymous 10 months ago

I'm also wondering about the nature of this rule, does anybody have idea what it should do?

* html #content{ width: 75%; padding-left: 0; margin-top: 0px; padding: 6px 10px 10px 10px;}

because in the default version, just one line above it would already be defined as:

#content{ width: 75%; padding-left: 0; margin-top: 0px; padding: 6px 10px 10px 10px;}

I have a feeling, Internet Explorer is guilty :)

#7 Updated by Marius BALTEANU 10 months ago

In my environment, the #content loads first with 100% width and right after, the sidebar which makes the screen "shakes".

#8 Updated by Anonymous 10 months ago

Marius BALTEANU wrote:

In my environment, the #content loads first with 100% width and right after, the sidebar which makes the screen "shake".

Marius, I looked into it, so I'm attaching alternative patch without element swapping in base.html.erb to see if this one will not spawn shaking, the previous patch must be reverted before applying this one.

The alternative method could as well be to set #main a flow-direction: row; instead of row-reverse and than add order: 1; for the #content and order: 2; for the #sidebar, but I guess this isn't needed now, because we only have 2 containers, but it could be useful if we re to spawn an additional tile under the sidebar in the future for instance, else in the favor of less rules, it is imho just be simpler to reverse it. :)

#9 Updated by Marius BALTEANU 10 months ago

Thanks Max for updating the patch, it works well with row-reverse.

I agree with Bernhard, the sidebar width (220px) is too small even for small screens, please see the below screenshot from my 13 inch screen:

1. width: 220px;

What do you think if we keep the existing width: 22% rule and we add a max-width of 280px or 300px?

#10 Updated by Anonymous 10 months ago

I think width: 22% will anyway often exceed it's maximum limitation of max-width, maybe we could try min-width: 220px and max-width: 280px or 300px?
In theory sidebar's content should then expand the sidebar in width within these margins a bit depending on what it contains. :)

#11 Updated by Anonymous 10 months ago

There is a gridbox design suggestion now pending for further discussion, so this ticket will be affected if we will decide to move #main-menu inside #main container, ultimately making this patch obsolete, because it will then become a 3 tiled gridbox instead of a flexbox. Please see #30451#note-6 for reference.

#12 Updated by Marius BALTEANU 10 months ago

  • Assignee deleted (Marius BALTEANU)

#13 Updated by Anonymous 9 months ago

I think it will be better to ignore #30451 for now, so attaching the patch with min-width and max-width rules for the sidebar, the content of the sidebar will now expand it up to 340px if contains long elements, if there is no need, it will reduce up to 220px. I think it's the most suitable resolution for everyone.
I also removed some redundant rules this time, like:

* html #sidebar{ width: 22%; }
* html #content{ width: 75%; padding-left: 0; margin-top: 0px; padding: 6px 10px 10px 10px;}

#14 Updated by Go MAEDA 9 months ago

Max Johansson wrote:

I think it will be better to ignore #30451 for now, so attaching the patch with min-width and max-width rules for the sidebar, the content of the sidebar will now expand it up to 340px if contains long elements, if there is no need, it will reduce up to 220px.

Thank you for the patch. Narrower sidebar than the current implementation should be nice for many people.

I tried the patch but I found that the sidebar hardly expands even if there is a custom query with a long name and the width of the browser window is around 1800px. With my Firefox, the width of the sidebar is 261px at 1024px window and 271px at 1800px window.

How can I see a sidebar with a width of 340px?

#15 Updated by Bernhard Rohloff 9 months ago

Go MAEDA wrote:

I tried the patch but I found that the sidebar hardly expands even if there is a custom query with a long name and the width of the browser window is around 1800px. With my Firefox, the width of the sidebar is 261px at 1024px window and 271px at 1800px window.

How can I see a sidebar with a width of 340px?

I can confirm the behavior you described. I think is an issue with the flexbox constraints.
Here is a modified version of Max's patch which prevents the sidebar from shrinking. Can you check if it's working now?

#16 Updated by Go MAEDA 9 months ago

I tested main-block-fixes-no-swap-min-max_no_shrink.diff.

When there is a query with a long name, the sidebar became too large even in small screens and the left side of the page is cropped.

#17 Updated by Marius BALTEANU 9 months ago

Go MAEDA wrote:

I tested main-block-fixes-no-swap-min-max_no_shrink.diff.

When there is a query with a long name, the sidebar became too large even in small screens and the left side of the page is cropped.

It is because of the missing width: 22%; rule.

#18 Updated by Bernhard Rohloff 9 months ago

The patch sets a min-width of 220px and a max-width of 340px for longer query names as it was constrained by Max in #30435#note-13.
By setting the width to 22% again we're loosing space on higher resolutions as the sidebar grows to its max-width even if there's no need to.
It can make a difference up to 120px which can give you an extra column in the issues list or an extra month on the gantt chart.

On the other hand I have to admit that 340px can be too big on small screens (37% @900px). It affects resolutions between 900px and around 1500px.
It just slightly affects the higher resolutions e.g. 1377px so should we consider to set the width only for smaller resolutions? What are your thoughts on this?

#19 Updated by Anonymous 9 months ago

Bernhard Rohloff wrote:

By setting the width to 22% again we're loosing space on higher resolutions as the sidebar grows to its max-width even if there's no need to.

What I tried to explain in #30435#note-10, but thanks for doing it more clearly. :)

On the other hand I have to admit that 340px can be too big on small screens (37% @900px). It affects resolutions between 900px and around 1500px.
It just slightly affects the higher resolutions e.g. 1377px so should we consider to set the width only for smaller resolutions? What are your thoughts on this?

I propose to either:
1. Create adaptive rules then where we d set max width of the sidebar for different screen sizes/resolutions like so:
@media screen and (min-width: 1400px) {#sidebar{max-width:340px;}
@media screen and (min-width: 1200px) {#sidebar{max-width:280px;}
@media screen and (min-width: 1000px) {#sidebar{max-width:220px;}

I don't have different displays at the moment to test this, perhaps I'll have to do it with setting other lower resolutions of my 1920x1080 native display, I will post an update when tests will be done then and post the patch.

2. Limit the length of the names of custom queries instead and then skip all the adaptive rules.
3. Or move queries away from the sidebar into a more lucky location and again skip all the adaptiveness, because frankly IMHO sidebar isn't the most successful place for the menu objects which allow for ridiculously ultra-long titles.

#20 Updated by Bernhard Rohloff 9 months ago

Max Johansson wrote:

I propose to either:
1. Create adaptive rules then where we d set max width of the sidebar for different screen sizes/resolutions like so:
@media screen and (min-width: 1400px) {#sidebar{max-width:340px;}
@media screen and (min-width: 1200px) {#sidebar{max-width:280px;}
@media screen and (min-width: 1000px) {#sidebar{max-width:220px;}

I don't have different displays at the moment to test this, perhaps I'll have to do it with setting other lower resolutions of my 1920x1080 native display, I will post an update when tests will be done then and post the patch.

Don't bother with fiddling around in your screen settings. You can test it easily with the builtin dev tools of you browser of choice. Nowadays they all have an option to test multiple screen sizes.
This is IMHO my prefered solution.

2. Limit the length of the names of custom queries instead and then skip all the adaptive rules.

I think this doesn't meet the requirements.

3. Or move queries away from the sidebar into a more lucky location and again skip all the adaptiveness, because frankly IMHO sidebar isn't the most successful place for the menu objects which allow for ridiculously ultra-long titles.

As im not a fan of the sidebar at all this would be my favorite solution. But it requires a lot more effort and discussion so I think this can't be an option for this feature, right now.

#21 Updated by Marius BALTEANU 8 months ago

Let move the discussions about the sidebar width to another ticket and commit the other changes that replace the float rules with flex. In this way, we fix an annoying bug on issue page that reproduces when the sidebar height is greater than the content height.

Current:

With the patch applied:

I'm attaching the patch that I would like to be committed, it contains the work done by Max and a small fix for the sidebar padding.

#22 Updated by Go MAEDA 8 months ago

  • Subject changed from Change #main block to a flexbox + additional minor fixes to it's direct children. to Change #main block to a flexbox + additional minor fixes to it's direct children

Marius BALTEANU wrote:

In this way, we fix an annoying bug on issue page that reproduces when the sidebar height is greater than the content height.

Nice. I have been bothered by the bug for years.

#23 Updated by Go MAEDA 8 months ago

  • Subject changed from Change #main block to a flexbox + additional minor fixes to it's direct children to Replace float rules with flexbox for content and sidebar block
  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed the fix. Thank you for working on this.

#24 Updated by Go MAEDA 7 months ago

The change made in r18033 breaks the layout of some pages on medium-sized screens.

To reproduce the problem, open /users page in a 1024px wide browser. The left side of the table is cropped and you cannot see the first name column.

#25 Updated by Marius BALTEANU 7 months ago

The below patch fixes the problem for me:

diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css
index 09bcd5b..79cf1d0 100644
--- a/public/stylesheets/application.css
+++ b/public/stylesheets/application.css
@@ -92,7 +92,7 @@ pre, code {font-family: Consolas, Menlo, "Liberation Mono", Courier, monospace;}
 #sidebar div.wiki ul {margin:inherit; padding-left:40px;}
 #sidebar div.wiki ul li {list-style-type:inherit;}

-#content { flex-grow: 1; background-color: #fff; margin: 0px; padding: 6px 10px 10px 10px; }
+#content { flex-grow: 1; background-color: #fff; margin: 0px; padding: 6px 10px 10px 10px; overflow-x: auto;}
 html>body #content { min-height: 600px; }
 * html body #content { height: 600px; } /* IE */

@Bernhard, is it ok for you this fix?

#26 Updated by Yuichi HARADA 6 months ago

Marius BALTEANU wrote:

The below patch fixes the problem for me:

[...]

@Bernhard, is it ok for you this fix?

Thank you for sharing the patch.
I confirmed that the layout on medium-sized screens( #30435#note-24, /issues and /time_entries pages) does not break by applying your patch.

#27 Updated by Bernhard Rohloff 6 months ago

I also tested the fix and it works for me, too.

#28 Updated by Go MAEDA 6 months ago

  • Status changed from Reopened to Closed

Committed the fix submitted as #30435#note-25 in r18135. Thank you.

#29 Updated by Antonio McDeal 2 months ago

To conclude the major point behind this issue, which never got resolved back then, here is a patch to try out as a reference to #30435#note-19

#30 Updated by Go MAEDA 2 months ago

  • Status changed from Closed to Reopened

#31 Updated by Go MAEDA 2 months ago

  • Status changed from Reopened to Closed

Antonio McDeal wrote:

To conclude the major point behind this issue, which never got resolved back then, here is a patch to try out as a reference to #30435#note-19

Thank you for posting the patch. I think the width of the sidebar should be discussed in another issue because the topic is really controversial as we experienced in this issue and it will be difficult to close this issue for a long time. In my opinion, I don't like the sidebar width with the patch applied because the width of the sidebar is wider than the current style at 1024px window width (I usually set the browser width to about 1000px - 1100px).

So, I close this issue. If anyone thinks the sidebar width should be improved with the approach suggested by the patch in #30435#note-29, please open a new issue.

#32 Updated by Antonio McDeal 2 months ago

I created a new issue #32037, please add it as related to this one, because part of important discussion was started in here. :)

#33 Updated by Go MAEDA 2 months ago

  • Related to Patch #32037: Constrain sidebar width on different resolutions added

Also available in: Atom PDF