fix: font size syncing #142
Conversation
We are grabbing the font size from the body element but setting it on the root document element. This means if the body in the FRA has `d2l-typography` applied then the font size is double scaled down.
|
I'm a bit concerned about keeping backwards compatibility with this. I could detect if the body in the FRA has |
I feel like this should be reasonably safe? FRAs would need to update their clients to get the new client-side behaviour, while it still looks like the client should be backwards-compatible - i.e. The host is providing a new-new property that older clients just won't know about. |
@grant-cleary I'm more concerned that people are relying on the font size being smaller then it should be in current FRA's (root html font size being |
|
FYI, @jameswklassen. |
plugins/sync-font/client.js
Outdated
| @@ -1,5 +1,5 @@ | |||
| export function clientSyncFont(client) { | |||
| return client.request('font').then(font => { | |||
| document.documentElement.style.fontSize = font.size; | |||
| document.documentElement.style.fontSize = font.sizeRoot ?? font.size; | |||
There was a problem hiding this comment.
Kinda thinking this might want to be
if (document.body.classList.contains('d2l-typography')) {
document.documentElement.style.fontSize = font.sizeRoot ?? font.size;
} else {
document.documentElement.style.fontSize = font.size;
}
There was a problem hiding this comment.
@grant-cleary I'm more concerned that people are relying on the font size being smaller then it should be in current FRA's (root html font size being
17.1pxvs20px). This current change will mean that font sizes will "get bigger" ifd2l-typographyisn't applied to the FRA's body class. Could cause stylistic changes that aren't desired. Although that would only happen if they update theifrauclient in their FRA.
Ah, fair. Yeah, I'm always a bit less concerned when teams need to make a conscious choice to upgrade, as (hopefully) they would look at the changes to figure out whether this would impact them. But you're right that in that case this would be an impactful change, so I'm also not at all opposed to your proposed solution of checking specifically for d2l-typography as a safeguard.
| @@ -1,10 +1,13 @@ | |||
| export function hostSyncFont(host) { | |||
| host.onRequest('font', () => { | |||
| const computedStyle = window.getComputedStyle(document.body); | |||
There was a problem hiding this comment.
To me, this seems like the bug right here? Like in our LMS pages, we output this:
html {
font-size: 20px;
}And that value changes when the user changes their font size preference in account settings. So could we just always grab it from the root and always set it on the root and ignore the body completely?
There was a problem hiding this comment.
Yeah, this is what started the font syncing discussion. In Portfolio we want to reflect the font size preference inside our iFrame.
We weren't sure if others depend on the current behaviour (getting computed style from <body> rather than <html>).
If it makes sense that the style comes right from <html> (and it doesn't break existing usage), then it might be cleanest to always get the computed style from the root
const computedStyle = window.getComputedStyle(document.documentElement);There was a problem hiding this comment.
I'm ok with treating this as a bug. I'm less worried after thinking about the fact that teams have to upgrade to get the new version of ifrau anyways and would hopefully test and see the difference. I can make the adjust to just grab the html font size and not introduce the new sizeRoot stuff if that works for everyone.
| family: computedStyle.fontFamily, | ||
| size: computedStyle.fontSize, | ||
| visualRedesign: visualRedesign | ||
| family: bodyComputedStyle.fontFamily, |
There was a problem hiding this comment.
Still using body to grab the font family. Isn't used but have it for compatibility.
|
🎉 This PR is included in version 0.39.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
We are grabbing the font size from the body element but setting it on the root document element. This means if the body in the FRA has
d2l-typographyapplied then the font size is double scaled down (the root document font size was around17.1px, since the LMS side root is20pxbut the body hasd2l-typographyso the body computed size if17.1px)