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

Set a default form_id for the subscribe pattern for testing #1689

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

calebeby
Copy link
Member

Conversation here: #1679 (comment)

@gerardo-rodriguez an alternate solution would be to make the twig file itself have its own default value for the form ID, but I don't know if that is the best because it would mean that the form id would be duplicated on the page if you included the subscribe component multiple times on the same page. Do you know if there is a way to make a twig prop required, so that it'll throw an error or something if the prop is missing?

@changeset-bot
Copy link

changeset-bot bot commented Mar 28, 2022

⚠️ No Changeset found

Latest commit: f89d83d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Mar 28, 2022

Deploy Preview for cloudfour-patterns ready!

Name Link
🔨 Latest commit f89d83d
🔍 Latest deploy log https://app.netlify.com/sites/cloudfour-patterns/deploys/6241e5f6c331ba0008f01212
😎 Deploy Preview https://deploy-preview-1689--cloudfour-patterns.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@calebeby calebeby changed the title Set a default form_id for the subscribe pattern for testing Set a default form_id for the subscribe pattern for testing Mar 28, 2022
@calebeby calebeby mentioned this pull request Mar 28, 2022
9 tasks
@gerardo-rodriguez
Copy link
Member

an alternate solution would be to make the twig file itself have its own default value for the form ID, but I don't know if that is the best because it would mean that the form id would be duplicated on the page if you included the subscribe component multiple times on the same page.

@calebeby I agree, I'd be concerned the form ID would be duplicated on the page if the component existed multiple times on the same page.

Do you know if there is a way to make a twig prop required, so that it'll throw an error or something if the prop is missing?

I am not aware, but also Twig isn't my forte so there might be something I'm not aware of.

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks, @calebeby!

@calebeby calebeby merged commit c82e75d into v-next Mar 28, 2022
@calebeby calebeby deleted the fix-form-id-subscribe-pattern branch March 28, 2022 18:14
@gerardo-rodriguez
Copy link
Member

an alternate solution would be to make the twig file itself have its own default value for the form ID, but I don't know if that is the best because it would mean that the form id would be duplicated on the page if you included the subscribe component multiple times on the same page.

@calebeby I agree, I'd be concerned the form ID would be duplicated on the page if the component existed multiple times on the same page.

Do you know if there is a way to make a twig prop required, so that it'll throw an error or something if the prop is missing?

I am not aware, but also Twig isn't my forte so there might be something I'm not aware of.

@calebeby Maybe you could do something with defined? Maybe render a message instead of the component if a given prop is not defined? 🤔

@calebeby
Copy link
Member Author

Oh yeah that might be helpful. IDK if it's worth it, but we could probably make a error pattern that is very visible/flashy and then go through all our patterns and make them render the error pattern if any of their required props are missing

@gerardo-rodriguez
Copy link
Member

That's an interesting idea! I can create a GH issue and we can leave it in the backlog as a nice-to-have as I imagine it'll be a lower priority through this initial phase of work.

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.

2 participants