Bug Fix: Date Picker Sass units incompatibility

[bignamehere]

https://github.com/USDA-FSA/fsa-style/blob/ba2b35c8e1b375079dd5a5ef6fa21c71b669568f/src/stylesheets/components/_fsa.date-picker.scss#L265

[atdiff]

@bignamehere running it with fsa-style@2.5.3 works great but using version 2.7.3 results in the error.

[atdiff]

@bignamehere I tested with 2.6.0 and same error there, so 2.5.3 and prior work fine. Those changes to _fsa.date-picker.scss were introduced as a part of 2.6.0 in July 2021. I'm guessing we haven't ran a build with that 2.6.0 version (or newer) till now which is exposing the issue.

[bignamehere]

With a recommendation from @whatnextjamie I looked into calc(). Using calc() instead of parentheses works in some instances, but not all of them. I updated node-sass to the latest version, because I wasn't seeing any errors related to this issue or others. From there, I updated SCSS files that tried to use division with a "/" with calc() and this seems to have worked, but calc() doesn't seem to render the appropriate results with addition and multiplication, and I am not sure why.

But calc() does allow for incompatible units to be added together, or at least it is not throwing any errors after the updates I have made. There looks to be about 20 SCSS files that are affected by these changes, but we need to take caution with any updates to FSA-Style and ultimately Nexus.

Additionally, I found a separate calc() mixin within what looks to be part of the uswds vendor files. So when using the native calc() this mixin was probably being run instead, and not performing any calculations. I updated this mixin to uswds_calc() and might need to retry the calc() everywhere it was failing previously.

@atdiff since there is no way for me to replicate the errors you initially were receiving, how can we best move forward with testing the single line update for this file? we basically need this line of code to be...

background-size: calc($size-medium + .2rem);

I can make this single change, but then you might immediately get another error with another instance of something related to SASS.

Thoughts?

[bignamehere]

Also, I found this referenced on Stackoverflow, and it might also be something we might need to look into...

We might need to change out all SASS variables that are included within a calc() and turn them into strings to be interpreted correctly.

[whatnextjamie]

@bignamehere I'm looking into this further, but I don't believe using calc() is necessary everywhere. Sass expressions can also be unit-clean, so background-size: $size-medium + .2; would probably return the desired result as well without build errors. Another thing I wanted to point out is that dividing using / is being depreciated in Sass (since / is used as a separator in CSS), so we'll need to use calc() for those, or math.div().

[bignamehere]

@whatnextjamie I tried to use both calc() and math.div() but only calc() was working.

I would receive these errors on build...

I think the issue has something to do with FSA-Style still using the legacy node-sass... https://stackoverflow.com/questions/69925082/invalid-css-after-padding-math-expected-expression-e-g-1px-bold-wa

[bignamehere]

@whatnextjamie & @atdiff I made a lot of progress by updating from node-sass to sass (DART SASS). I am no longer seeing any deprecation warnings or error messages. Feel free to pull down this branch and see if you experience the same thing. You will need to npm uninstall node-sass. I also updated grunt and grunt-sass to the latest versions, although I am not sure if grunt-sass is actually being used now.

[whatnextjamie]

@bignamehere Nice! I'm having a ton of trouble getting sass to compile, but it seems to be how scss-lint uses Ruby gems. I did find that scss-lint is being depreciated, so that's something we'll need to look into at some point: https://github.com/sds/scss-lint#readme. I'll pull down your branch to see if that helps, but I think my problem is local.

[whatnextjamie]

@bignamehere And yeah, it looks like math.div() is only compatible with Dart Sass. I'm trying to wrap my head around what's being used where, but I'd like to eventually remove whatever isn't being used, and keep documentation on it all.

[bignamehere]

@whatnextjamie The easiest way to can understand that is to look at the Diff by clicking on the checking ID above (eg bf2efb9)

One of the major things that I couldn't figure out was that the SASS errors kept returning on certain files, and I forgot that the Gruntfile.js was copying a bunch of files from node_modules of the USWDS (a legacy version), and overwriting them in the stylesheets. So I commented out that copy, as we will not be updating these USWDS directly from NPM anyway, and maintaining them separately.

The rules I would like to follow moving forward are as follows:

• Always use calc() when there is a arithmetic using SASS variables (This allows for certain units that don't match to be used)

• Use parenthesis for all other arithmetic and to separate order of operations

• Always use math.div() for division and NOT calc() with a /

I believe I have tried to follow all of the above rules for the latest checkin listed above.

[whatnextjamie]

@bignamehere Yeah, I guess my comment didn't make a ton of sense. I was actually referring to figuring out what we're using for linting, if grunt-sass if being used, etc. I think that would make troubleshooting easier. As far as the bullet points, I think we should follow Sass best practices according to the docs. This is just an example:

The method used will depend on the variable's unit. calc() will work if they are different units, but so will just declaring the unit on the variable only. calc() will need to be used for calculations where you must have different units, though. For example, px per second for animations. There you're actually dividing pixels by seconds, and need to use calc() with a /.

I'd like to not introduce a lot of extra headaches, and follow the docs instead, since CSS & Sass aren't as black & white as other languages. We can definitely have preferred approaches, but saying "always" is kinda scary with CSS.

[atdiff]

@bignamehere @whatnextjamie I've upgraded locally to the current 2.7.3 version and tested that out with the one line change mentioned above. I had to wrap the variable like this to get it to go through the webpack/css process: background-size: calc(#{$size-medium} + .2rem); I'll follow up with an email of some visual differences for you all to review.