-
Notifications
You must be signed in to change notification settings - Fork 3
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
"Get Weekly Digests" pattern #1679
Conversation
🦋 Changeset detectedLatest commit: c9216fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for cloudfour-patterns ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
heading "Get Weekly Digests" | ||
text "Get Weekly Digests" | ||
text "Email" | ||
textbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to using the Input Group with an Input component and this value changed. Before when I was using a vanilla <input>
, it read textbox "Email"
. Now it only reads textbox
. The "name" value seems to have gone away?
I reviewed both the vanilla and Input Group with the Input component and I was unable to notice a difference in the markup so I'm not sure why this changed. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerardo-rodriguez I tried to debug this locally, I logged the markup using:
console.log(await page.evaluate(() => document.body.innerHTML));
right before the snapshot.
It looks like the id
/for
attributes aren't getting set correctly.
<label class="c-subscribe__form-input-label" for="subscription-email-">
Email
</label>
<div class="o-input-group c-subscribe__form-input-group">
<input type="email" class="c-input c-subscribe__form-input" id="subscription-email-null" placeholder="Your
Email Address" autocomplete="home email" required="">
Looks like we are missing the form_id
prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I logged the markup using
Great idea! We should add this “recipe” tip somewhere in the Pleasantest docs. It wasn’t obvious I could do that. LOL
Looks like we are missing the form_id prop?
Oh, odd! Let’s create a GH issue for this. Can you help me with that? If not, no worries, I can do so later this morning.
Thanks for following up, @calebeby!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylersticka @spaceninja I'm not sure why CI isn't running after my last commit, but this PR is ready for another review! I have addressed all feedback. I did leave a question for @calebeby, but I don't think that's a blocker for this PR. |
Co-authored-by: Tyler Sticka <tyler@cloudfour.com>
Co-authored-by: Tyler Sticka <tyler@cloudfour.com>
…dfour.com-patterns into feature/disclosure-widget * 'feature/disclosure-widget' of github.com:cloudfour/cloudfour.com-patterns: Update src/objects/input-group/input-group.twig Update src/components/subscribe/subscribe.twig
@tylersticka Thanks for your feedback! 🎉 I have addressed the latest round of feedback. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will defer approval to @cloudfour/dev since I feel less qualified to review the JS and testing stuff!
@calebeby @spaceninja Can either of you help out with a final dev review? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, and works as described
Overview
This PR introduces a Subscription Choices component.
Screenshots
Testing
Preview: https://deploy-preview-1679--cloudfour-patterns.netlify.app/?path=/story/components-subscribe--default-story
Accessibility
General
With both keyboard and mouse inputs, clicking/activating the "Get Weekly Digests" link should:
:focus
onto the email input fieldIf the form is visible, hitting Escape on the keyboard should:
:focus
to the "Get Weekly Digests" linkTheming
Known issues
I was unable to figure out how to set the form background color for the "dark alternate" theme. Any ideas @tylersticka?(This has been resolved)