-
Notifications
You must be signed in to change notification settings - Fork 353
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
Course Theme: Add gravatar hovercard in comments #7333
base: trunk
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,9 @@ | |||
document.addEventListener('DOMContentLoaded', () => { |
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.
Could we follow the same whitespace rules in this JS file as we do for Sensei?
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.
Yapp, good point. I've updated it here 07d099e
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.
Oh, looks like the spaces get removed by this pre-commit hooks #3596
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.
Oh, looks like the spaces get removed by this pre-commit hooks #3596
Is there any P2 or similar that documents how to integrate hovercards that we could link to in the description? |
course/functions.php
Outdated
* | ||
* @return string The <script> tag for the enqueued script with defer attribute. | ||
*/ | ||
function defer_parsing_of_js( $tag, $handle ) { |
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.
What do you think about bumping the minimum supported WP version to 6.3 and specifying a loading strategy instead?
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've used loading strategy and updated the version number here 4ee873d. Very nice idea Donna! Thank you!!
@Imran92 Can you provide an update here? |
Thanks for the ping Donna. Some theme PRs got left out when we moved to other project. I'll update this PR as per the suggestions above. Adding it to my to-do |
Yap you are absolutely right. Looks like gravatar-hovercard library fixed it themselves on 0.5.8 release to not load the hovercard on mobile! So I've just pointed to the updated version here c3f3eba. On my testing, it looks like it doesn't detect resizing, so we have to reload the page to understand that it's working. Thanks!! |
I found their readme very helpful and contained all the required information. I'm adding it to the PR description. |
Changes proposed in this Pull Request:
Gravatar hovercard library documentation
https://github.com/gravatar/hovercards/blob/0.5.8/README.md
Testing instructions:
Screen.Recording.2023-08-28.at.5.46.58.PM.mov
Related issue(s):
#7306