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

Geolocation: only enqueue dashicons when necessary. #10108

Merged
merged 1 commit into from
Sep 12, 2018

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Sep 6, 2018

Fixes #10089

Changes proposed in this Pull Request:

Only enqueue the dashicons when we need them, when the icon must be displayed on the page, and only once.

Testing instructions:

  1. Check out this branch.
  2. Go to https://wordpress.com
  3. Pick your site, and start writing a new post. Add a location to your post thanks to the option in the sidebar.
  4. In your theme or in a functionality plugin, add the following:
function jeherve_enable_geo(){
       	add_theme_support( 'jetpack-geo-location' );
}
add_action( 'after_setup_theme', 'jeherve_enable_geo' );
  1. Make sure the little location icon appears alongside the post location when you view that post you just wrote.
  2. Make sure the dashicons.css file is not loaded on your home page or on any other page, when logged out of your admin account.
  3. Make sure there are no errors in your logs.

Proposed changelog entry for your changes:

  • Geo Location: only enqueue Dashicons when necessary.

This is not as pretty as using the proper `wp_enqueue_scripts` hook,
but it helps only enqueue the dashicons when we need them, when the icon must be displayed on the page, and only once.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Feature] Geo Location labels Sep 6, 2018
@jeherve jeherve added this to the 6.6 milestone Sep 6, 2018
@jeherve jeherve self-assigned this Sep 6, 2018
@jeherve jeherve requested a review from griffbrad September 6, 2018 14:28
@jeherve jeherve requested a review from a team as a code owner September 6, 2018 14:28
@jetpackbot
Copy link
Collaborator

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@griffbrad griffbrad closed this Sep 11, 2018
@griffbrad griffbrad reopened this Sep 11, 2018
Copy link
Contributor

@griffbrad griffbrad left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 12, 2018
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve merged commit ba3a757 into master Sep 12, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 12, 2018
@jeherve jeherve deleted the fix/dashicons-geolocation-10089 branch September 12, 2018 16:44
jeherve added a commit that referenced this pull request Sep 14, 2018
jeherve added a commit that referenced this pull request Sep 24, 2018
jeherve added a commit that referenced this pull request Sep 25, 2018
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Geo Location [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants