Patch #30168

Wrap "splitcontentright" and "splitcontentleft" containers with a flexbox

Added by Max Johansson about 1 month ago. Updated about 21 hours ago.

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

0%

Category:UI
Target version:4.1.0

Description

Currently as far as I saw, "splitcontentright" class and "splitcontentleft" class containers are only actually wrapped into a "splitcontent" class inside an "issue" container class in the issue view on any issue page.
Everywhere else, like e.g. Home page, Project overview page, the "splitcontentright" class and "splitcontentleft" are not wrapped inside "splitcontent" wrapper (besides this wrapper doesn't even exist there). Instead they rely on independent width set to 49% and a respective floating rule depending on the side they re on.

1. Such solution leads to inconsistent UI behavior when you will be resizing page horizontally (or content block in particular). I think they should be wrapped into a "splitcontent" everywhere where possible, in order to get rid of independent 49% of the width rules and just set the parent wrapper container to a flexbox (which i think "splitcontent" class already was), then splitting, which will keep exact half of the available space will be automatically.

2. Wrapping them will also make selection of splitcontainers so much easier in CSS (if you re to make global rules to every split container).

3. Similar problem was with "list-left" and "list-right" containers on My Page, as they again rely on independent 49% of width rather than being wrapped into a flexbox (or gridbox if you will).

splitcontent-wrapper-fix.diff Magnifier (15 KB) Max Johansson, 2018-12-13 05:03

splitcontent-wrapper-fix-unix.diff Magnifier (7 KB) Max Johansson, 2018-12-13 06:26

splitcontent-wrapper-fix-unix-spacesfix.diff Magnifier (7.02 KB) Max Johansson, 2018-12-15 10:36

use_splitcontent_in_my_page.patch Magnifier (1.35 KB) Marius BALTEANU, 2019-01-14 13:42

use_splitcontent_in_my_page_responsive_fix.patch Magnifier (808 Bytes) Max Johansson, 2019-01-15 08:59

splitcontent_responsive_fix.diff Magnifier (719 Bytes) Max Johansson, 2019-01-16 09:45

splitcontent_responsive_fixed_fix.patch Magnifier (684 Bytes) Max Johansson, 2019-01-19 10:46

Associated revisions

Revision 17790
Added by Go MAEDA 9 days ago

Wrap "splitcontentright" and "splitcontentleft" containers with a flexbox (#30168).

Patch by Max Johansson.

Revision 17810
Added by Go MAEDA 2 days ago

Add the splitcontent* classes to my page layout (#30168).

Patch by Marius BALTEANU.

Revision 17835
Added by Go MAEDA about 21 hours ago

Fix the styles for the responsive (#30168).

Patch by Max Johansson.

History

#1 Updated by Max Johansson about 1 month ago

Uploaded a patch (my first one, hopefully I did all correctly...), if I did overlook some elements which should probably also be wrapped, please do inform :)
To test: Go to my page or my account page and resize your browser window horizontally, you will see that split blocks will now keep the same distance from each other.
edit: preview doesn't seem to work potentially due to windows (CR-LF) line breaks.

#2 Updated by Max Johansson about 1 month ago

Uploading alternative one just in case (hopefully this one generated well).

#3 Updated by Go MAEDA about 1 month ago

Thanks for the patch.

Max, do you know if the patch works fine with Internet Explorer 11? The website Can I use states that there are many flexbox bugs in IE 11.

#4 Updated by Max Johansson about 1 month ago

Go MAEDA, works just fine for me on my IE 11.741.17134.0 which is bundled with Windows 10. According to known issues list, it doesn't seem like I used any of the flexbox related CSS rules which should cause problems on that browser. IE 10 is however what could be worth to test it on, so I'll do some tests and report back asap. If I'll spot bugs I'll then alter the CSS a little to make it even IE 10 friendly.
If flex boxes is a bad idea, there is still a way to make it behave exactly the same, but without using flex box rule.

#5 Updated by Go MAEDA about 1 month ago

  • Target version set to Candidate for next major release

I think flexbox is not bad at all if it works with IE 11. Thank you for checking the compatibility with IE.

IMHO it is OK to include the patch in Redmine 4.1.0.

#6 Updated by Marius BALTEANU about 1 month ago

I like the idea to replace float left/right rules with some more modern CSS rules.

I've added as watchers the guys from plan.io who worked a lot on UI changes in the last years, maybe they can help us with reviewing the proposed patch.

#7 Updated by Marius BALTEANU about 1 month ago

One more thing, Max, please replace in your patch the tab characters with spaces.

#9 Updated by Go MAEDA 11 days ago

  • Target version changed from Candidate for next major release to 4.1.0

For consistency, I think it should be fixed as Max suggested. Setting the target version to 4.1.0.

#10 Updated by Felix Gliesche 10 days ago

I checked the patch and I really like it. It adds consistency and replaces the floating layout with a flex-box grid, which I personally find very elegant and "state-of-the-art".

@Go MAEDA: is IE 11 officially the minimum IE requirement?

#11 Updated by Go MAEDA 10 days ago

  • Tracker changed from Defect to Patch
  • Subject changed from Inconsistent wrapping of "splitcontentright" and "splitcontentleft" containers to Wrap "splitcontentright" and "splitcontentleft" containers with a flexbox

Felix Gliesche wrote:

@Go MAEDA: is IE 11 officially the minimum IE requirement?

As far as I know, the minimum IE requirement is not officially defined. But I think we don't have to support IE <11 because those are no longer supported by Microsoft (they still support IE 9 and 10 on old Windows servers but the market share of those browsers should be very small).

#12 Updated by Felix Gliesche 10 days ago

Go MAEDA wrote:

Felix Gliesche wrote:

@Go MAEDA: is IE 11 officially the minimum IE requirement?

As far as I know, the minimum IE requirement is not officially defined. But I think we don't have to support IE <11 because those are no longer supported by Microsoft (they still support IE 9 and 10 on old Windows servers but the market share of those browsers should be very small).

Yes, I agree. The patch still looks good in IE10 for example, it is just stacking like on mobile. In case we want to support IE < 11 we could add prefixes to the flex box attributes and get it running there. But yes, the market share is very small, so I guess it is really not necessary.

#13 Updated by Go MAEDA 9 days ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed the patch. Thank you for improving Redmine.

#14 Updated by Marius BALTEANU 7 days ago

  • Status changed from Closed to Reopened
  • Assignee changed from Go MAEDA to Max Johansson

Max, what do you think if we do not mix the splitcontent class with other elements that do not have splitcontentleft and splitcontentright classes? I'm referring at My Page layout where the children of splitcontent have list-left and list-right ids (and to add those classes is a little bit too complicated).

I think that the above patch will make the layout more consistent:

diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css
index fa59d6a..f4b48a4 100644
--- a/public/stylesheets/application.css
+++ b/public/stylesheets/application.css
@@ -441,10 +441,11 @@ div.square {
 .contextual input, .contextual select {font-size:0.9em;}
 .message .contextual { margin-top: 0; }

-.splitcontent {overflow: auto; display: flex; flex-wrap: wrap;}
+.splitcontent, #my-page {overflow: auto; display: flex; flex-wrap: wrap;}
 .splitcontentleft, #list-left {flex: 1; margin-right: 5px;}
 .splitcontentright, #list-right {flex: 1; margin-left: 5px;}
-#list-top {width: 100%;}
+#list-top {flex: 2; flex-basis: 100%;}

Also, what do you think about replacing width: 100% with flex:2; flex-basis: 100%;?

#15 Updated by Max Johansson 7 days ago

1. The patch already wraps #list-left and #list-right with .splitcontent block in app/views/my/page.html.erb so .splitcontent, #my-page {overflow: auto; display: flex; flex-wrap: wrap;} will spawn a double flexbox, and I am not sure if it's good :(
I think if we re to change from:

<div id="my-page">
<div class="splitcontent">

To

<div id="my-page" class="splitcontent">

and then also give #list-left and #list-right the .splitcontentleft and .splitcontentright classes in addition to their ids, it will be much better, I'm just not sure how complicated this can be because I haven't yet researched it, so I'll do some tests in the evening. :)
I think we should avoid styling the repeating elements by their IDs as much as we can, if we re to achieve 100% True Jedi Consistency and also enable the ability for these types of containers to be reusable and with the same expected behavior everywhere, so plugin developers who are to design their own views could reuse classes and wouldn't be confused by their behavior.

I also think in case of my page, IDs are there strictly only for binding purposes in jQuery to enable things like block swapping (for the most part atleast).

2. I agree with replacing #list-top {flex: 2; flex-basis: 100%;} and also think it's a nice addition. Maybe we might also need to introduce .splitcontenttop class as well, as again, it could be useful to reuse for plugin developers. Many things could be reusable actually, like tool tips and numerous other little things. But that's another due story I guess :)

#16 Updated by Marius BALTEANU 7 days ago

Max Johansson wrote:

1. The patch already wraps #list-left and #list-right with .splitcontent block in app/views/my/page.html.erb so .splitcontent, #my-page {overflow: auto; display: flex; flex-wrap: wrap;} will spawn a double flexbox, and I am not sure if it's good :(
I think if we re to change from:
[...]

To

[...]

Sorry Max, I've totally forgot to attach to my note the full patch that contains also the removal of the new splitcontent div from my page.

and then also give #list-left and #list-right the .splitcontentleft and .splitcontentright classes in addition to their ids, it will be much better, I'm just not sure how complicated this can be because I haven't yet researched it, so I'll do some tests in the evening. :)
I think we should avoid styling the repeating elements by their IDs as much as we can, if we re to achieve 100% True Jedi Consistency and also enable the ability for these types of containers to be reusable and with the same expected behavior everywhere, so plugin developers who are to design their own views could reuse classes and wouldn't be confused by their behavior.

I also think in case of my page, IDs are there strictly only for binding purposes in jQuery to enable things like block swapping (for the most part atleast).

2. I agree with replacing #list-top {flex: 2; flex-basis: 100%;} and also think it's a nice addition. Maybe we might also need to introduce .splitcontenttop class as well, as again, it could be useful to reuse for plugin developers. Many things could be reusable actually, like tool tips and numerous other little things. But that's another due story I guess :)

Totally agree with you and having a second thought on this, it was much easier than I expected to add the splitcontent* classes to my page layout, please view the attached patch (which obsoletes my previous proposal and covers most of our requirements).

#17 Updated by Max Johansson 6 days ago

I tested and it looks good :)
Maybe responsive.css also might need minor corrections.

#18 Updated by Marius BALTEANU 6 days ago

Max Johansson wrote:

I tested and it looks good :)
Maybe responsive.css also might need minor corrections.

Thanks for testing it and for pointing out the changes for responsive.css. I tried to apply your patch on top of my patch, but it doesn't apply cleanly.
Anyway, I think that we should write the styles for responsive only one time instead of duplicating them and we need as well to set margin-right to 0 (otherwise you will have a 5px margin for splitcontentleft element).

I think the patch should look something like that:

  .splitcontentleft, .splitcontentright, .splitcontenttop {
    width: 100%;
    flex: auto;
    margin-left: 0;
    margin-right: 0;
  }

#19 Updated by Max Johansson 5 days ago

Marius BALTEANU wrote:

Thanks for testing it and for pointing out the changes for responsive.css. I tried to apply your patch on top of my patch, but it doesn't apply cleanly.

Is it probably because of the version conflict? use_splitcontent_in_my_page_responsive_fix.patch needs to be applied over the splitcontent-wrapper-fix-unix-spacesfix.diff patch, else it might not find lines :)

Anyway, I think that we should write the styles for responsive only one time instead of duplicating them and we need as well to set margin-right to 0 (otherwise you will have a 5px margin for splitcontentleft element).

I think it's a much cleaner solution too, so adding a patch for it now, it should also be applied over already applied splitcontent-wrapper-fix-unix-spacesfix.diff
After that, I think we re generally done with this issue for now and should apply these hotfixes, commit and re close this issue asap. :)

#20 Updated by Marius BALTEANU 5 days ago

  • Assignee changed from Max Johansson to Go MAEDA

#21 Updated by Marius BALTEANU 4 days ago

Patches to be committed:

1. use_splitcontent_in_my_page.patch which will add the splitcontent* classes to my page layout.
2. splitcontent_responsive_fix.diff which fixes the styles for the responsive. The patch still doesn't apply cleanly on current trunk + my patch, but the changes are straightforward.

#22 Updated by Go MAEDA 2 days ago

Marius BALTEANU wrote:

Patches to be committed:

1. use_splitcontent_in_my_page.patch which will add the splitcontent* classes to my page layout.
2. splitcontent_responsive_fix.diff which fixes the styles for the responsive. The patch still doesn't apply cleanly on current trunk + my patch, but the changes are straightforward.

Committed use_splitcontent_in_my_page.patch.

Max, could you update splitcontent_responsive_fix.diff so that it can be applied cleanly?

#23 Updated by Max Johansson 2 days ago

Remade this patch against very recent trunk, should be working, if not, please inform me (it looks the same as last one, though :( ) :)
I reverted and tested to apply it, it worked for me to apply it cleanly to the most recent development trunk.

#24 Updated by Go MAEDA about 21 hours ago

  • Status changed from Reopened to Closed

Committed Max's splitcontent_responsive_fixed_fix.patch. Thanks.

Also available in: Atom PDF