style(actor): fix scrolling

[wrycu]

• fix skill scrolling in default theme

#1351

[sebastiansIT]

In my opinion, there is a better way fixing this without all these explicit height calculations. Give me some try with flexbox and overflows. Thanks.

[wrycu]

This PR adds padding; it doesn't do any calculations. The calculation lines were touched because they had trailing spaces and my IDE is set up to strip that.

I'd (personally) suggest another PR to refactor the code, if that's what you want.

[Esrin]

I agree that we can do this better (and the system's CSS overall has become a bit of a frankenstein's monster like everything else), but unfortunately even in my new system which uses entirely flexbox, there's no avoiding 1 calc call on height due to a quirk in how Foundry sheets are styled. The alternative is to completely override some core Foundry styling which gets a bit breaky across Foundry updates.

That said, the only adjustment we need to make here is a tweak to the current height calc. Let me have a quick look.

[Esrin]

So on closer inspection, the change that broke the existing calculations was this one: #1322

As it altered the height of the header. To cope with it, we really ought to have a fixed header height (preferably in a CSS variable) and just make it scrollable as the Mandar theme has done.

Then for any content height calculation all you need is something like:

  height: calc(100% - (#{$sheet-header-height} + 0.5rem));

In which the 0.5rem adjustment accounts for the Foundry window-header / top bar which throws things out a bit.

All that said, digging through the SCSS files to find this has made me realise what an absolute state they're in... so I'm tempted to say let's just adopt the Mandar theme globally and give ourselves a fresh styling start from there. :P

[Esrin]

All things considered, here's my proposed alternative:
#1366

[wrycu]

@Esrin that's fine, but it doesn't solve the bug in this theme (and making the other the default doesn't remove the existing.)

[Esrin]

@Esrin that's fine, but it doesn't solve the bug in this theme (and making the other the default doesn't remove the existing.)

Nope, but removing the existing will remove the existing. The linked PR is just step 1.

[wrycu]

If you are planning on a phased roll-out, this PR should still be approved. If you aren't planning on a phased roll-out, we should update your PR to be more aggressive in removing the default theme @Esrin