Configurable opacity in loading box

[podliashanyk]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Test results

   10 files  1 050 suites   37m 57s ⏱️
  532 tests   531 ✅  1 💤 0 ❌
5 320 runs  5 310 ✅ 10 💤 0 ❌

Results for commit bac11aa.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.29%. Comparing base (f08319f) to head (bac11aa).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1090   +/-   ##
=======================================
  Coverage   81.29%   81.29%           
=======================================
  Files         141      141           
  Lines        5089     5089           
=======================================
  Hits         4137     4137           
  Misses        952      952           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hmpf]

LGTM but @elfjes should see it before I merge

[elfjes]

For now, I like the approach here. It's simple and provides flexibility. I'm not a huge fan of the api though. Setting a variable through a tw class looks.... strange. It ís good for ad hoc things that you do once in a while though. Such as now. If we start using this more often, we may want to think about a more tailwind-idiomatic way. Perhaps a small tw-plugin?

Not something we have to think about now however.

One thing that we should consider. The new opacity for the modals is (to me) only subtly different from the default. I would be ok with changing the (default) opacity instead of making it configurable. This would change the opacity of the incident list while loading, but since the change is quite subtle, that's fine.

If we do keep the configurability, it needs some docs

[podliashanyk]

For now, I like the approach here. It's simple and provides flexibility. I'm not a huge fan of the api though. Setting a variable through a tw class looks.... strange. It ís good for ad hoc things that you do once in a while though. Such as now. If we start using this more often, we may want to think about a more tailwind-idiomatic way. Perhaps a small tw-plugin?

I don't exactly agree that it is strange. Tailwind in no way provides a full set of utilities to cover all the functionality of CSS. That's why they added the possibility to extend existing utilities with arbitrary values. It is common enough that they recommend it as one of the strategies for customizing styles (https://tailwindcss.com/docs/adding-custom-styles#using-arbitrary-values), and they even allow for dropping wrapping with var() when using TW arbitrary values (as one of the handy shortcuts for arbitrary values). Furthermore this approach is in line with TW's philosophy regarding atomic design so I don't see any problem with it.

... Perhaps a small tw-plugin?

I don't see equally as simple way to replace the arbitrary value usage with a plugin. Do you have some thoughts about the implementation?

Not something we have to think about now however.

One thing that we should consider. The new opacity for the modals is (to me) only subtly different from the default. I would be ok with changing the (default) opacity instead of making it configurable. This would change the opacity of the incident list while loading, but since the change is quite subtle, that's fine.

Sounds reasonable to change the default to 0.5 to me. The question is whether we still want to keep the ability to customize? 🤔

If we do keep the configurability, it needs some docs

Good point, will add if we decide to go ahead with this PR!

[elfjes]

I don't exactly agree that it is strange. Tailwind in no way provides a full set of utilities to cover all the functionality of CSS. That's why they added the possibility to extend existing utilities with arbitrary values. It is common enough that they recommend it as one of the strategies for customizing styles (tailwindcss.com/docs/adding-custom-styles#using-arbitrary-values), and they even allow for dropping wrapping with var() when using TW arbitrary values (as one of the handy shortcuts for arbitrary values). Furthermore this approach is in line with TW's philosophy regarding atomic design so I don't see any problem with it.

Some people say that tailwind is just syntactic sugar for inline style. While I do not agree with those people completely, one of the main benefits of using tailwind vs inline style is that tailwind is more concise and readable due to having less syntax. Once you reintroduce syntax for ad-hoc classes, you start to lose some of that readability. This is fine if used sparingly, since tailwind cannot accommodate for every situation, but if it becomes a pattern, that's a smell to me. In that case we may want to spend some effort in making a solution that preserves tailwinds readability and idioms, which could be a tailwind plugin. I am definitely not saying that we've reached that point (yet) but it's something to keep in mind for the future.

... Perhaps a small tw-plugin?

I don't see equally as simple way to replace the arbitrary value usage with a plugin. Do you have some thoughts about the implementation?

Oh it's definitely not as simple as what we do now, but I don't think it's that more complex either. I have only glanced at tailwind's plugin system, but it seems that all we'd need to do is create the desired classes (.loading-box-opacity-10, .loading-box-opacity-20, etc using a for loop) in js and given them to tailwind in the config. Again, don't do this now, but it might be useful in a future where we'd want to use those classes more than once or twice.

Not something we have to think about now however.
One thing that we should consider. The new opacity for the modals is (to me) only subtly different from the default. I would be ok with changing the (default) opacity instead of making it configurable. This would change the opacity of the incident list while loading, but since the change is quite subtle, that's fine.

Sounds reasonable to change the default to 0.5 to me. The question is whether we still want to keep the ability to customize? 🤔

My initial reaction would be: yagni