Skip to content

Conversation

@msigley
Copy link

@msigley msigley commented Jan 30, 2018

Reworked metabox implementation to:

  • actually verify nonces
  • validate post saves to prevent excess google geocode api calls
  • support additional location data fields via the 'obj_location_post_meta' filter

Added really basic theme template and template variable system.
Only template currently defined is "obj-map-location-pin-content.php" for overriding the location pin content. This is useful to list other types of locations besides local businesses, like events or classes, and gives the ability to provide an alternative schema.org markup.

Added resave check to admin to prevent geocoding on every post save.
Added separate Geocode API Key setting to allow for use of Google API key restrictions.

Matthew Sigley added 12 commits January 29, 2018 14:10
Reworked javascript to eliminate a ton of unnecessary info in the CDATA.
Added nonce verification and post save validation to location save handling.
Reworked layout of metabox to better support multiple fields.
Added geocode api key setting to allow for better google maps api key security settings.
Close open infoWindows on marker and map click.
Fixed code formating. Converted space indentation to tabs.
Reorganized assets.
Removed unused files.
Updated direct file access prevention.
Changed name of CDATA varable from 'data' to 'objGoogleMapsData' to prevent conflicts with other scripts and plugins.
Removed unused location icon setting.
Use local icon resources instead of undocumented google images.
Added active location icon functionality.
Added title to map markers.
@msigley
Copy link
Author

msigley commented Feb 9, 2018

Ok. This change set turned out to be much larger than I originally expected. I was using this plugin in a project and just upstreamed all of my changes to make the plugin more extensible and work in our use case.

Enough things have changed here that this warrants a version 2.0, which I reflected in the code. The plugin now has much improved UX in the admin area and on the map.

@msigley msigley changed the title Metabox rework and added customization options. Version 2.0 Feb 9, 2018
@msigley
Copy link
Author

msigley commented Feb 12, 2018

I added two minor bug fixes I discovered during my testing of this change set.

@wesleycole
Copy link
Contributor

@msigley thanks for submitting! Let me look through this.


var markerLatLng = new google.maps.LatLng({ lat: numLat, lng: numLng }),
toCenterDistance = google.maps.geometry.spherical.computeDistanceBetween( center, markerLatLng );
console.log(toCenterDistance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

var numLat = parseFloat(lat);
var numLng = parseFloat(lng);
(function( $ ) {
(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
Copy link
Contributor

Choose a reason for hiding this comment

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

@msigley what is the point of this?

@clifgriffin
Copy link

@msigley Thanks for the contribution man.

So our biggest concern here is that you stripped out package.json, npm, gulp, ES6, etc.

That makes it difficult for us to merge this in. Is it possible you could pull in your changes without redoing the JS stack?

@msigley
Copy link
Author

msigley commented Mar 21, 2018

I dont use npm, gulp, ES6, etc in my dev environment so I had no way to test my changes to those files. I also noticed those files were of date from previous commits.

What if I add main.min.js files for the admin and public js assets? Or could one of you implement gulp on this new js stack?

@clifgriffin
Copy link

Hmm, npm and gulp are a breeze to get started with, especially in a pre-configured project like this one.

You basically just run npm install, and then run gulp to compile the assets. It takes care of compiling ES6 into JS, minificiation, et al. You would do this in terminal / cmd. It should work fine cross platform as long as you have Node installed.

As it is, we aren't really comfortable merging in a PR that deconstructs our tool stack. :-/

@msigley
Copy link
Author

msigley commented Mar 22, 2018

I'll look at it and get back to you. Personally I think using post build tools in open source projects like this adds a barrier of complexity for contributions, Shopp's JS was proof of this for me, but to each their own I guess.

Honest question, is there a benefit to using npm and glup over using something like the closure compiler service?
https://closure-compiler.appspot.com/home

I thought gulp was mostly used in large enterprise projects to combine and compile a number of JavaScript files together to reduce the number of JS requests on the page. In 2018 though, I think a minifying proxy server like Cloudflare combined with HTTP/2 support to combine requests into a single connection makes this enterprise gulp workflow obsolete.

@clifgriffin
Copy link

Most Open Source projects include a tool stack these days, whether it's webpack, grunt, gulp, etc. WP core includes one, for example.

The problem with Shopp was that it didn't have a built in stack. It was something Jon did himself on his own machine using an esoteric process. I added gulp to Shopp to fix this problem, so I believe the latest 1.3.x/master branches have this built in.

Using gulp (or one of its competitors) in projects like this is very much the norm. While there are SaaS style solutions to the problem, the benefit of including it in the project is that anyone with Node installed can initialize it in their local dev environment and get exactly the same results.

And it's often a lot more than just minifying / concatenating. It's compiling ES6 / CoffeeScript to native JS. SASS to CSS, etc.

You can't rely on everyone to implement their own stack or SaaS to handle those functions.

Especially when it's roughly two commands to get the same stack running for any developer who close the project :-)

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.

3 participants