New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Theme Checks] Added in a check for theme block settings validation #620

Closed

AribaRajput wants to merge 6 commits into main from ariba/presets-settings

Contributor

AribaRajput commented Nov 27, 2024 •

edited

Loading

What are you adding in this PR?

First part of #602

Adding in a theme check for checking theme blocks for valid preset and default setting keys. The preset and default setting keys (and the presets of any theme block preset settings) must match the settings keys configured in either the preset settings or the nested theme block settings.

What did you learn?

A lot about presets, sections and theme blocks and how they are configured. That was the trickiest part of all of this (managing the types, knowing how to grab nested information, etc).

Before you deploy

This PR includes a new checks or changes the configuration of a check
- I included a minor bump changeset
- It's in the allChecks array in src/checks/index.ts
- I ran yarn build and committed the updated configuration files
  - If applicable, I've updated the theme-app-extension.yml config

I included a minor bump changeset
My feature is backward compatible

I included a patch bump changeset

AribaRajput force-pushed the ariba/presets-settings branch 17 times, most recently from b95fc84 to 866ea6c Compare

December 2, 2024 18:30

AribaRajput marked this pull request as ready for review

December 2, 2024 18:37

AribaRajput requested review from charlespwd and a team

December 2, 2024 18:37

charlespwd reviewed

View reviewed changes

.changeset/chilled-bugs-juggle.md Show resolved Hide resolved

charlespwd reviewed

View reviewed changes

.changeset/chilled-bugs-juggle.md Outdated Show resolved Hide resolved

charlespwd reviewed

View reviewed changes

packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts Outdated Show resolved Hide resolved

charlespwd reviewed

View reviewed changes

packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts Outdated Show resolved Hide resolved

charlespwd reviewed

View reviewed changes

packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts Outdated Show resolved Hide resolved

charlespwd reviewed

View reviewed changes

packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts Outdated Show resolved Hide resolved

charlespwd requested changes

View reviewed changes

Contributor

charlespwd left a comment

Position information can't be ignored for this check. We'll probably need to rewrite it and iterate over each blocks separately rather than push all of them in the same bucket.

AribaRajput force-pushed the ariba/presets-settings branch 2 times, most recently from b20d7f0 to 0a7932b Compare

December 4, 2024 15:06

AribaRajput marked this pull request as draft

December 5, 2024 21:20

AribaRajput force-pushed the ariba/presets-settings branch from 05d6e6c to 07c3295 Compare

December 5, 2024 21:25

AribaRajput force-pushed the ariba/presets-settings branch 4 times, most recently from 3ef48eb to eee6ce7 Compare

December 19, 2024 21:01

AribaRajput added 3 commits

December 19, 2024 17:18


          Added in a check for theme block settings validation

bb87775


          theme block theme check

625d79e

Finished tests

Added changeset


          Reworked

6f90bcc

AribaRajput force-pushed the ariba/presets-settings branch from eee6ce7 to 6f90bcc Compare

December 19, 2024 22:18

AribaRajput requested review from charlespwd and albchu

December 23, 2024 15:25

AribaRajput force-pushed the ariba/presets-settings branch from 8f6b402 to 8745150 Compare

December 23, 2024 20:27

AribaRajput changed the title ~~Added in a check for theme block settings validation~~ [Theme Checks] Added in a check for theme block settings validation


          Added in default setting validation

a262ff6

AribaRajput force-pushed the ariba/presets-settings branch from 8745150 to a262ff6 Compare

January 9, 2025 18:28

AribaRajput added the #gsd:43130 label


          Reformatted tests to have clearer structure

299e675

AribaRajput force-pushed the ariba/presets-settings branch from a005288 to 299e675 Compare

January 21, 2025 19:02


          Refactored theme check

b7a7bd6

AribaRajput force-pushed the ariba/presets-settings branch from 9d91791 to b7a7bd6 Compare

January 23, 2025 20:10

AribaRajput commented

View reviewed changes

packages/theme-check-common/src/checks/valid-preset-and-default-settings/index.ts

+                      const currentPath = [...path, blockIndex.toString()];
+                      for (const [settingId, _] of Object.entries(block.settings ?? {})) {
+                        const node = nodeAtPath(ast, [...currentPath, 'settings']);

Contributor Author

AribaRajput Jan 27, 2025

Need to nest this one level deeper - the node is highlighting all of settings rather than the specific id key

Contributor

aswamy commented Jan 30, 2025

Closing this PR for this one: #742

Thank you Ariba for the initial PR ❤️

aswamy closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels