Closed MoritzLost closed 1 year ago
Actually I would go even further, why we need Sass at all, it is 2021, CSS is powerful alone. And for me CSS > SASS for small projects.
I think it would be a major change to have CSS alone. We should leave SASS for now, maybe reconsider for a tobii v3.
So we have several options for a fix of this issue:
To be honest, I can't decide right now which way to go. If you have a clever opinion on this, go ahead :)
@midzer Drop the function. It's kind of superfluous, there are so many good ways to get relative font sizes. And looks like the function is doing the exact opposite. It takes a pixel value and the base font size just to output an em
unit that will result in the same pixel size regardless of base font size. So what's even the point of using em
? If you want a hardcoded size in pixel why not just use px
?
If you want to support relative font sizes, two good options:
em
units.calc()
with a custom property for the base font size. This would allow users of the lightbox to specify the base font size the same way they can with SCSS variables right now.I agree, that this function should be removed and we should use em
for fonts and rem
for buttons width and height.
So:
p.s. I believe we should remove "Sass" as soon as possible, because it gives no benefits, but gives troubles during development.
I would be happy about any PR to get progress going on in this issue.
I see this warning still appears. I think the most straightforward solution is adding calc():
@function em($pixels, $base-font-size: 18) {
@return #{calc($pixels / $base-font-size)}em;
}
Not sure if it works in all sass compilers.
I sent a PR if you want to check it.
Are we 100% sure we want to do it in wrong way? This function should be removed.
p.s. Thank you for PR, but I am trying to look at broader picture.
I can push commit, which removes this function, if everybody agrees.
I didn't know it was a bug, but if my PR has been helpful in solving an old issue, that's ok.
In my opinion you shouldn't use calculation functions like these for em values. If switching from pixel to em values in the source code is not possible right now, the pull request should use sass math compiler's like @MoritzLost suggested, since the values basically never change and calculating it inside CSS provides no benefits, because the font-size can already be changed by adjusting the parent containers font-size.
Finally some progress with https://github.com/midzer/tobii/commit/eb2d1de806699104333596e6d040de7d32a086e4.
Too bad I did not manage to recover the original 1:1 computed px values from the demo. There is probably some tweaking necessary.
@midzer if you would add prefix "#89 " to commit message, github.com automatically attach log to related issue. Very helpful for everybody.
Finally some progress with https://github.com/midzer/tobii/commit/eb2d1de806699104333596e6d040de7d32a086e4.
@midzer In my opinion, trying to force specific pixel dimensions with values like 0.27778em
or 2.22222rem
is not a good idea. If I understand correctly, those values are set to generate fixed pixel values based on the previous base size of 18px
. However, you've changed the --tobii-base-font-size
to 1rem
, which will be 16px
on most devices. Don't try to force specific pixel weights, and don't assume that everyone will use the default base font size. The purpose of the relative units and the --tobii-base-font-size
variable should be the ability to scale the entire UI with a single variable (or browser setting). Use simple fractions (like 1.25em
, 0.25
etc) to create a nice, even, relative scales. Then I can use something like --tobii-base-font-size: 1.25rem
to scale up the entire UI if I want to, and the UI scales nicely by default with the font size defined in the user's browser. It doesn't matter if the relative units don't create round pixel values, most devices have a DPR > 1 and support sub-pixel rendering, anyway.
@MoritzLost Thanks for your explanation!
Wanna open a PR with those em
tweaks and we can have a review together?
@midzer Sure: https://github.com/midzer/tobii/pull/105
I've converted all the values to approximately match the previous values (based on the base-size of 1rem). If the base size isn't modified, all of those values now result in clean pixel values. If it is, they will scale nicely along the base size.
Is it significant that ~50px appear in both --tobii-slide-max-height
and in top: 2.77778em;
of tobii__slider
? In this case, that should probably be another property so it can be changed with a single custom property.
The
em
function in the_functions.scss
file uses the deprecated slash syntax for division:See Breaking Change: Slash as Division for details. Importing the
tobii.scss
causes a warning in Dart Sass and will stop working at all in Dart Sass 2.0. For now I'm just importing the pre-compiledtobi.min.css
instead – though it would be nice to able to import the source files instead, this way our build pipeline rules (including autoprefixer rules) will apply to it as well.Here's an updated version of that function:
Though this will break all older sass compilers (node-sass in particular). If you want to support those, there are other options. Bootstrap replaced the slash division with a custom divide function that will work for all compilers.
A broader picture
Another option would be to drop that function altogether and just use
em
normally. Right now the function doesn't respect the actual--base-font-size
anyway, so I'm not sure why it's there or what it's doing. Using native em units, relative units like vw/vh or just sticking with pixels would be preferable in my opinion.