search

openedx / edx-platform

The Open edX LMS & Studio, powering education sites around the world!
https://openedx.org
GNU Affero General Public License v3.0
7.17k stars 3.83k forks source link

Fix/Ignore: [Bourbon] [Deprecation] rem is deprecated and will be removed in 5.0.0. #33571

Open DanielVZ96 opened 10 months ago

DanielVZ96 commented 10 months ago

This warning is appearing 165 times during provisioning. It clutters logs and hinders dev experience (https://github.com/openedx/edx-platform/issues/32888).

A decision needs to be made on whether all rem mixin usages are swapped with normal rems, or just ignore the warnings.

AC

devkhan commented 1 month ago

assign me

devkhan commented 1 month ago

Since the warning is about deprecation, should we upgrade the js/npm package? And how do we ensure other dependencies don't break because of this?

feanil commented 1 month ago

@devkhan Since the rem from bourbon is being removed, we can't update the package until after we stop using the feature that is going to be removed. If you're going to take on this ticket, I suggest you figure out how to switch from the bourbon rem function to the normal rem bulit into modern CSS. To ensure we don't break anything you'll have to do visual spot checks of the UIs that use the relevant CSS.

devkhan commented 1 month ago

@feanil I think I got confused by your statement there. There is a rem in Bourbon v4, which is for converting px to rem units. But there is a rem function built into modern CSS (which is very recent), but it's for calculating the remainder (like % in python), and is completely different from the rem and em units used in CSS.

I think the solution will be to replace bourbon's rem with plain rem units, e.g. rem(24) -> '24rem'. Bourbon also suggests the same thing in it's migration guide.

Let me know if I'm on the right track and I can get started on that. Although, this very likely can have consequences in terms of UI (as font sizes units are changed). I think it should not cause any issues, but I'm not sure.

feanil commented 1 month ago

@devkhan correct, moving from the rem function to the standard rem unit and then visually checking UIs to make sure we didn't cause major issues.