search

flatironinstitute / stan-playground

Run Stan models in the browser
Apache License 2.0
36 stars 1 forks source link

Replace inline styling with CSS classes where possible. #133

Closed jsoules closed 4 months ago

jsoules commented 4 months ago

An attempt to use semantically-meaningfully-named CSS classes over inline styling. Most styling is in localStyles.css at the top level, though I'm not too attached to that; it might make more sense to break them down by the area of the app they impact, but our situation isn't sufficiently complex to justify an elaborate CSS solution right now in my mind.

Some class names may be subject to revision if I've misconstrued the function they serve in the actual layout. There are also a few cases where I'd like to combine some of the stylng in separate css files into localStyles.css, but I wanted to discuss a bit first whether that's the right thing to do.

One area to remark is that we have style sheets for several different types of tables with no clear distinction about what's used where. I removed the draws-table-2.css stylesheet since no existing component was actually using any of it, and I renamed table1.css to something a bit more semantic that I think identifies its function, but there's more that could be done here.

Many instances of inline styling still exist; these are mainly either manually computed positioning for elements, or including a "color" style as a parameter for one of the toolbars. I think we would benefit from moving away from absolute positioning & manually computed dimensions, but that's outside the scope of the current PR.

magland commented 4 months ago

@WardBrian @jsoules I have concerns about this. I recognize that it's best to put styling in external .css files (and we should do that), but it bothers me that we're maintaining the styles globally. I understand that global styling makes sense when we're talking about colors or padding that apply to multiple components. But what you are doing here is is making global entries for specific components, IMO breaking the modularity of the component system.

Also see this: https://github.com/flatironinstitute/stan-playground/issues/112

I think there is a difference between layout concerns using css and styling concerns (colors, etc). And those should be treated separately.

Since this is already merged I think we can go forward, but I may propose reverting a lot of this to address the above concerns.