search

lockedata / lockeutils

Utils R package for the Locke Data team :art: :chart:
Other
1 stars 0 forks source link

Check logic in `zzz.R` for font checking #9

Closed DaveParr closed 5 years ago

DaveParr commented 5 years ago

Hey, sorry to be a bad news bear, but I was just doing some testing and I think I spotted something:

re: https://github.com/lockedata/lockeutils/blob/274c6be7aba767c48886f11fc649ff48f8c96490/R/zzz.R#L11

!any(grepl("Contrail|Roboto", fnt$FamilyName))
!any(grepl("Contrail|Roboto|Notfont", fnt$FamilyName))
!any(grepl("Notfont|Contrail|Roboto", fnt$FamilyName))

Shouldn't the bottom two return TRUE? I definitely don't have a font called Notfont :slightly_smiling_face:

If i understand the intent of that line correctly, it should check if any of those 3 font names are missing, and then trigger the startup message? Seeing as I've included a font that doesn't exist it should trigger TRUE to show the message? I think it's only matching partial portions of the string, it's probably just a regex wierdness.

Also, do we potentially need to test for Roboto Thin as well/rather than Roboto? In the event that the user has partially installed the return from lockeutils::import_roboto(), but only installed Roboto, but not Roboto Thin, this check will pass, but the call to the thin font in theme_ld() might fail as it is looking for something that isn't there?

https://github.com/lockedata/lockeutils/blob/274c6be7aba767c48886f11fc649ff48f8c96490/R/theme_ld.R#L25

maelle commented 5 years ago

Hey, sorry to be a bad news bear

Do NOT be sorry to help improve the package! 😺

maelle commented 5 years ago

Btw at the moment technically Contrail One isn't needed... but it should soon be needed hence the messaging. 🙈