Bootstrap grid

[smacker]

1. All pages use bootstrap grid
2. Extensive documentation for the most complicated part of 100% with scrollable content on annotation page.
3. Only errors component has custom width. I couldn't find a good variable in bootstrap for it.
4. Index page isn't centered by vertical anymore. I don't want to introduce hack with position absolute because then I'll have to hard-code more heights. But I'm open to suggestions.

[dpordomingo]

Wow! I like it!

Everything is great but the maths behind the code panels positioning :(
It would be better if we could solve it in a way around, or moving the problem to a more local place.

[dpordomingo]

I understand the problem, but I think it should be faced with variables.
Header and footer sizes could be defined in vars, so that vars could be used here.
That way, changing something in another place, would not break this component.
Making the header fixed will mean another thing: it could break if its content is changed, but it should be a local problem, instead of an external problem.

[smacker]

Thanks for looking in it. As I explained before variables don’t really save us from breaking layout. But moving some numbers to variables just for better readability might be a good idea.
Could you please provide a small snippet for your idea, so I will be sure I got what you mean right?

[dpordomingo]

yes, things will eventually break ;) I only tried to break things in a limited scope. Let's see if this can do it:

(code below this line, is kind of pseudocode 😉 )

header.less

$headerInnerHeight: 33px;
$headerMargingBottom: 10px;
$headerHeight: $headerInnerHeight + $headerMargingBottom;
.header { height: $headerInnerHeight; margin-bottom: $headerMargingBottom;}

footer.less

$footerInnerHeight: 22px;
$footerMargingBottom: 5px;
$footerHeight: $footerInnerHeight + $footerMargingBottom;
.header { height: $footerInnerHeight; margin-bottom: $footerMargingBottom;}

code-panel.less

@import 'header.less'
@import 'footer.less'
.code-panel {
    height: 100%;
    padding-top: $headerHeight;
    padding-bottom: $footerHeight
}

That way the code-panel does not depend on hardcoded values, but in values defined in places where that values refer to.

[smacker]

yeah. correct. Only 1 thing height: $headerInnerHeight is confusing in header/footer. Because it's not what it does. I will proceed without this rule, but with variable+comment.

[dpordomingo]

I'm not sure if I understood you when you said

$headerInnerHeight is confusing in header/footer. Because it's not what it does.

since my example was only pseudocode, it didn't consider the whole component definition 💃 , so I'm open to other implementations;

[dpordomingo]

Mmmmm... better than before thanks!... but I feel it does not follow my intentions:

If the positioning of the code panel depends on the header and footer, it should be guaranteed that it will depend on both. To do so, the header and footer should be defined in a way that the breaking changes affect them, instead of to other page components

Anyhow, talking with @carlosms he realized about a cleaner way to define the relations between the header-codepanel-footer using flexbox and avoiding maths. Let's analyze his proposal before considering mine.

[carlosms]

My proposal is to use a single Grid element, and handle height with flexbox.

I just created a PR against @smacker's fork to show the proposed changes more easily.
This is more a quick suggestion than a final PR, there might be things than can be handled better.

smacker#4

[smacker]

@bzz @carlosms @dpordomingo I would suggest to merge it as it is and make another issue for improvement. We have 2 other PR directly or indirectly depended on it already. And I think fixing math issue will take quite much time.

[bzz]

@smacker is there any reason not to merge work done by @carlosms under smacker/pull/4 first?

[smacker]

@bzz because in my opinion, it shouldn't be done like that. We can have a discussion in another issue/PR.

[bzz]

Ok, I think indeed, it would be nice to have a discussions in PRs to this repo rather then to the forks, if @dpordomingo and @carlosms agree.

At the same time - I would prefer no to loose the work that @carlosms has already done.

Would it be possible to merge it, than this one, and re-work from there in subsequent PRs?

Or would something like b00818c be enough to address the @dpordomingo concern from above #45 (review) and move on merging this?

[smacker]

I proposed some solution for @dpordomingo feedback IRL, he likes it :)
But I would like to implement it in separate PR (need to make sure it really works, check for edge cases).

[dpordomingo]

If we're pragmatic, this PR works. I tested it on my computer, and it succeeded. It's true that I don't like that maths in behind, and we can think of an alternative, but blocking this changes till we find a proper alternative seems unnecessary.
Since @carlosms changes were made from @maxim ones, I think they won't be lost if we merge this now, and create an issue pointing to the other PR smacker#4.
Do you agree @bzz ?

[dpordomingo]

As said in my prev comment, LGTM.
Could you @smacker create an issue pointing to Carlos PR to avoid forgetting it? Thanks!

[bzz]

LGTM. What @dpordomingo says sounds very reasonable!

Let's merge, as soon as

Could you @smacker create an issue pointing to Carlos PR to avoid forgetting it? Thanks!

[smacker]

placeholder for issue: #58