linnarsson-lab / loom-viewer

Tool for sharing, browsing and visualizing single-cell data stored in the Loom file format
BSD 2-Clause "Simplified" License
35 stars 6 forks source link

Migrate from Bootstrap for layout to Flexbox #31

Closed JobLeonard closed 8 years ago

JobLeonard commented 8 years ago

... while maintaining sanity in the face of CSS hell.

Ok, so I spent three freaking days (and counting) trying to do something that should be simple, except it's WebDev so it's insanely arcane:

Holy moly what a clusterf#@$@# that turns out to be... I'm not going to repeat all the problems here, but it boils down to old-school CSS sometimes ignoring sizes of elements on the page, and sometimes not, and in general having all kinds of modes that are in conflict with the browser doing its best to combine it all. Another issue is that I don't know which kind of CSS Bootstrap already applies to our elements, which is likely to lead to conflicts when I try to set something manually.

And this isn't a pointless exercise in code clean-up: if the only way to deal with this is ugly work-around hacks, any change to the related code or layout of entangled components will result in shit breaking again later. Especially the last bit, about not having to worry about canvas size as long as we define a clear container is important.

So people have been complaining about these in WebDev for decades now, and recently created the Flexbox layouting standard to make the whole thing more consistent and predictable. This is a web-standard, supported by every recent browser, with a very consistent rule-set that will make my life a lot more sane:

https://css-tricks.com/snippets/css/a-guide-to-flexbox/

I propose we switch to using this, and relegate Bootstrap to only skinning the UI elements (and perhaps even getting rid of that in the long-term).

JobLeonard commented 8 years ago

(this is also why loom.linnarssonlab.org is laid out so strangely at the moment)

JobLeonard commented 8 years ago

Migrating the heatmap will require a bit of trickery, but solutions exist:

http://stackoverflow.com/questions/27240894/leaflet-map-in-a-flexbox-layout

http://stackoverflow.com/questions/36362849/render-leaflet-map-after-flexbox-calculates-height

https://github.com/tombatossals/angular-leaflet-directive/issues/950

JobLeonard commented 8 years ago

This looks like it works so I'm closing this, but if there's any bugs on your side let me know!

So I just fixed most of the CSS - except for the parts hardwired into Sparkline and Heatmap at the moment, basically. Easier than expected; I was assuming some freaky edge-case would ruin everything, but flexbox has really consistent and predictable behaviour.

Because of that, we don't need to use weird hacks to work around the quirks of float: left, which it replaces. This allows us to get rid of margins and padding we've used to make the divs align with the navbar, or the canvas stay to the right of the sidebar. We only use them for spacing right now. Other than that, margins, padding and orders default to zero, and every div is basically a nested flexbox:

* {
  margin: 0;
  padding: 0;
  border: none;
}

/* aligns elements left-to-right */
.view {
  display: flex;
  flex-direction: row;
  flex: 1 1 auto;
}

/* aligns elements top-to-bottom */
.view-vertical {
  display: flex;
  flex-direction: column;
  flex: 1 1 auto;
}

.sidepanel {
  min-width: 200px;
  max-width: 300px;
  padding-left: 10px;
  padding-right: 20px;
  flex: 1 1 auto;
}

(I gave the sidepanel a min-width of 200px, for smaller screens; any screen > 900 px should have a 300px wide sidepanel)

Having the same default makes the whole thing more predictable, on top of which we can then add CSS classes or local overrides with style={{ ... }}. Makes the whole much simpler.

Also, I've stripped out some classes that appeared to be for older versions that no longer apply (at least, I don't think we have a footer any more, do we?).

Anyway, now that it's guaranteed that our CSS layout behaviour is consistent, we can reason more clearly about what causes heatmap to misalign (which is the bug I'll look into now).