-
Notifications
You must be signed in to change notification settings - Fork 7
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
Adding News Letter Notice to Draw Attention #51
Conversation
…om/nsquared-team/draw-attention into implementing-news-letter-component
Hello @gnarza @NatalieMac @tylerdigital here is a quick demo to go over the implementation of the news letter metabox component. Thank you for your time. |
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.
Hello @NatalieMac here is the latest demo update for the requested changes. Thank you for your time! |
} | ||
}); | ||
|
||
document.addEventListener("keydown", function (event) { |
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'm going to go ahead and pass this since I don't anticipate it causing issues for this case, but generally, you want to attach this event listener fro the escape key when the modal opens and then remove it from the body once the modal is closed.
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.
Thank you! I'll keep that in mind.
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.
public/includes/da-newsletter.php
Outdated
} | ||
|
||
public function enqueue_meta_box_assets() { | ||
wp_enqueue_style( 'custom-meta-box-styles', $this->plugin_directory . '/assets/css/custom-meta-box-styles.css', array(), DrawAttention::VERSION ); |
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.
@Majed-Habli we need to namespace scripts/styles since this will cause conflicts if any other plugin has a style custom-meta-box-styles
, so this will need to become da-custom-meta-box-styles
wherever it's registered and enqueued
public/includes/da-newsletter.php
Outdated
|
||
public function enqueue_meta_box_assets() { | ||
wp_enqueue_style( 'custom-meta-box-styles', $this->plugin_directory . '/assets/css/custom-meta-box-styles.css', array(), DrawAttention::VERSION ); | ||
wp_enqueue_script( 'news-letter-js', $this->plugin_directory . 'assets/js/news-letter.js', array(), DrawAttention::VERSION ); |
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.
same here, da-
prefix
</div> | ||
<div class='outer-content'> | ||
<p>Subscribe now! Get 20% Coupon</p> | ||
<button id='openModalButton' onclick='openModal()'> <span>" . __( 'SUBSCRIBE', 'draw-attention' ) . "</span> </button> |
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.
Let's change to Subscribe
and apply text-transform: uppercase
to achieve the all caps
public/includes/da-newsletter.php
Outdated
|
||
global $wp_meta_boxes; | ||
|
||
if ( 'da_image' == $post_type ) { |
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.
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.
Thank you @tylerdigital for the explanation! i'll start making it a habit to do that instead 👍.
…om/nsquared-team/draw-attention into implementing-news-letter-component
@tylerdigital Updated the code as requested. 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.
thanks, looks good!
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 have some small requests
- In the screen options, fix small typo "News Letter" to "Newsletter"
- for the fonts used in all the Newsletter components, let's switch to a sans-serif font to match the figma more similarly
- the email font-color when entering my email should be darker it's difficult to see what I've entered
- the confirmation screen shouldn't take me to a new page, if it does then we need to make it simple to get back to the DA editor page/WordPress
And, my submission didn't go through to ActiveCampaign - is it hooked up yet?
Tested here: https://da-pr-51.sandbox.ssa.rocks/wp-admin
@gnarza, I've applied the requested modifications. Can you please check if the active campaign allows us to customize the confirmation page over on their site ? |
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.
- In the screen options, fix small typo "News Letter" to "Newsletter" ✅
- for the fonts used in all the Newsletter components, let's switch to a sans-serif font to match the figma more similarly ❌ still using the same fonts as before on sandbox
- the email font-color when entering my email should be darker it's difficult to see what I've entered ❌ still pale grey when inputting the email
- the confirmation screen shouldn't take me to a new page, if it does then we need to make it simple to get back to the DA editor page/WordPress ✅ I have to adjust it in ActiveCampaign for now I have it going to the pricing page
- submission ✅ submission went through successfully for me now
https://www.loom.com/share/ff90dcdfddbb4408b65099a5ef3c5753?sid=8295b2d1-6b64-4f53-bc12-0664b41e3e90
Tested on same sandbox with new pr zip https://da-pr-51.sandbox.ssa.rocks/wp-admin
@gnarza i tired running it, everything looks good. I think it might have been a cache issue or that the sandbox somehow wasn't fully loaded? |
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.
Looks like the font styling came through after hard reload on my end, and I also tested on another sandbox 👍🏻
Confirmation page/redirect page didn't open in a new tab though, but it seems that's expected behavior for these ActiveCampaign forms
https://app.asana.com/0/1202993069895285/1206073108876520/f