Skip to content
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.

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

Update validation settings templates to be consistent with the tutorial #134

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

aledalgrande
Copy link
Contributor

@aledalgrande aledalgrande commented Aug 30, 2024

Background

Related #103
Corresponds to https://github.com/Shopify/shopify-dev/issues/43285

Changing to a more detailed extension in all 4 flavors to be in line with the tutorial on shopify.dev.

Solution

Added a full solution making use of metafields and FunctionSettings and Section components.

Checklist

  • I have 🎩'd these changes
  • I have squashed my commits into chunks of work with meaningful commit messages

@aledalgrande aledalgrande requested a review from a team as a code owner August 30, 2024 22:47
@aledalgrande aledalgrande linked an issue Sep 3, 2024 that may be closed by this pull request
@avocadomayo avocadomayo marked this pull request as draft September 16, 2024 18:08
@avocadomayo
Copy link
Contributor

Converting this to a draft as we refactor some of the code examples

Copy link
Contributor

@alfonso-noriega alfonso-noriega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return (
<FunctionSettings onSave={onSave} onError={onError}>
<ErrorBanner errors={errors} />
<BlockStack gap="large">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove this wrapper BlockStack in a follow-up once we close https://github.com/Shopify/web/issues/124573. Applies to all other flavours

@avocadomayo avocadomayo marked this pull request as ready for review September 20, 2024 18:11
api.data.validation?.metafields?.[0]?.value ?? "{}",
);

if (!api.data.validation?.metafields) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be inferred by the const configruation up above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, configuration would never be null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but configuration.whatever would be

Copy link
Contributor Author

@aledalgrande aledalgrande Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to taste, my thinking is this is more generalizable, but you could also switch this to !configuration[firstId]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted shared data access to a var 😄

@avocadomayo avocadomayo merged commit 3966cce into main Sep 24, 2024
@avocadomayo avocadomayo deleted the alessandro/validation-settings-update branch September 24, 2024 22:54
This was referenced Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Templates improvements
4 participants