Add DeprecatedFonts theme check #967
Conversation
19e0a4b to
56b4f02
Compare
56b4f02 to
66da27d
Compare
packages/theme-check-common/src/checks/deprecated-fonts-on-sections-and-blocks/index.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // only read setting type if value is a deprecated font | ||
| async function checkBlocksHashForDeprecatedFonts( |
There was a problem hiding this comment.
I feel like this function and its array equivalent can be merged into one - they share most of their code. The for-loop just has to be modified a little so we check if it's an array of hash.
There was a problem hiding this comment.
yeah I do this in theme-graph to traverse both array and hash form with the same for loop.
I'm able to get the nodeAtPath in both forms too!
// Iterate over array entries or object entries depending on how the blocks are defined
const iterator = Array.isArray(preset.blocks)
? preset.blocks.entries()
: Object.entries(preset.blocks!);
for (const [keyOrIndex, block] of iterator) {
const nodePath = ['presets', i, 'blocks', keyOrIndex];
const node = nodeAtPath(ast, nodePath)! as ObjectNode;
// ...
}There was a problem hiding this comment.
Thanks for the feedback! As a ruby dev who doesn't know typescript, I don't always have the most refined typescript code. But I incorporated this approach and was able to consolidate :)
f17e5f8 to
c65aabf
Compare
packages/theme-check-common/src/checks/deprecated-fonts-on-sections-and-blocks/index.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/deprecated-fonts-on-sections-and-blocks/index.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/deprecated-fonts-on-sections-and-blocks/index.ts
Outdated
Show resolved
Hide resolved
1ecd52f to
2259a97
Compare
| message: string, | ||
| fullHighlight: boolean = true, | ||
| ) { | ||
| const node = nodeAtPath(ast, ast_path)! as ArrayNode; |
There was a problem hiding this comment.
I feel like this casting is dangerous given this method is extracted as a util 🤔?
Why not call context.report directly and do the casting when you need it instead?
Would remove the need for the fullHighlight thing?
reportWarning is pretty generic and it would feel weird to try to use that in a Liquid check, I'm not sure this deserves to be extracted?
There was a problem hiding this comment.
Fair point re: extracting, so, un-extracting :)
2259a97 to
8305695
Compare
8305695 to
a3a513d
Compare
What are you adding in this PR?
Video Demo:
Demo video: https://screenshot.click/17-29-7jiob-03dcd.mp4
Outdated:
This adds a check for section and block schemas to stop usage of deprecated fonts in:
We're doing this as part of the Monotype migration project which aims to move all deprecated fonts to a replacement by September.
What's next? Any followup issues?
Yes, settings_schema.json is most likely of all files to have fonts. We need to write a theme check for this. We may also, if possible, wish to add a check for json templates and settings_data.json to thoroughly warn against deprecated font usage.
What did you learn?
That there are no existing checks for
settings_schema.jsonor the other data files, so we don't have the building blocks for these.Before you deploy
changesetallChecksarray insrc/checks/index.tsyarn buildand committed the updated configuration filestheme-app-extension.ymlconfigchangesetchangeset