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

Improvements for validation settings template #140

Merged

Conversation

avocadomayo
Copy link
Contributor

@avocadomayo avocadomayo commented Sep 25, 2024

Background

Related #103
Follows #134

Solution

This PR does two things:

  • Fix a bug with the example validation settings app
    • We previously checked api.data.validation?.metafields to see if a metafield definition exists. However, this field is a bit misleading such that it evaluates to [] when the definition does not exist OR definition exists and no value is defined. As a result, we end up creating the metafield definition on each render
    • Add a metafieldDefinitions query to explicitly check for existing metafield definitions, and only create a new definition if there is truly no existing definition
  • Remove layout component BlockStack, no longer necessary since we provide the layout directly in FunctionSettings

Checklist

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

- Query to check for existing metafield definitions
- Remove layout component `BlockStack`, no longer necessary
Copy link
Contributor

@aledalgrande aledalgrande left a comment

Choose a reason for hiding this comment

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

Makes sense ✨

@avocadomayo
Copy link
Contributor Author

Hey @cpeddecord 👋 I moved applyMetafieldChange back to onChange for the number field. Sorry for the mix-up 🙏

@avocadomayo avocadomayo merged commit dfa4add into main Sep 30, 2024
@avocadomayo avocadomayo deleted the avocadomayo/validation-function-settings-improvements branch September 30, 2024 17:51
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.

3 participants