IE is not a modern browser #8768
Conversation
| @@ -30,6 +30,9 @@ const modernFontBrowsers = { | |||
| major: 11, | |||
| minor: 4, | |||
| }, | |||
| IE: { | |||
| major: 100, // specify impossible IE version, forcing passesRequirements to fail | |||
There was a problem hiding this comment.
Hrm, maybe the alternative is to cause passesRequirements to return false as the else to this clause?
https://github.com/learningequality/kolibri/blob/release-v0.15.x/kolibri/core/assets/src/utils/browserInfo.js#L21
There was a problem hiding this comment.
that's what I did at first, until I read that the docstring which says
It does this in a permissive way, whereby if no specification is made for a particular browser name, then it will return true.
There was a problem hiding this comment.
Yeah, I think the guy who wrote might have meant it to, because of this use of the same function:
There was a problem hiding this comment.
this makes sense to handle unknown or strange new browsers
There was a problem hiding this comment.
I think this is probably fine for now, but we can update the behaviour of passesRequirements when we move to using this explicit allowed list of supported browsers: https://github.com/learningequality/kolibri/blob/release-v0.15.x/packages/browserslist-config-kolibri/index.js#L1
|
@radinamatic would you mind verifying this fix for the IE font loading issue #5939? It should be present in the latest force-pushed version of #8756 |
|
True, I can confirm in the build from this PR that non-Latin fonts are not loading anymore by default on every refresh, and all language labels are present in IE11.
These issues were still extant in the latest build from #8756 that I tested yesterday, but I could repeat the test to be sure 👍🏽 |
nice! I did not expect that to be addressed yet 😅 |
Summary
The
passesRequirementsfunction was returningtruebecause IE was not included in the requirements.References
fixes #5939
Reviewer guidance
The behavior of
passesRequirementswas surprising at first, but it makes sense.That said, the solution feels like a hack. Is there a better way?
Testing checklist
PR process
Reviewer checklist
yarnandpip)