-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Geo Location Feature #123
base: develop
Are you sure you want to change the base?
Conversation
bf466fa
to
17d2c6c
Compare
--ep-c-medium-red: #d73c38; | ||
|
||
color: var(--ep-c-medium-red); |
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.
@burhandodhy can't we use any of the already existent EP colors?
public function set_i18n_strings(): void { | ||
$this->title = esc_html__( 'Geo Location', 'elasticpress-labs' ); | ||
|
||
$this->summary = '<p>' . __( 'Geo Location feature allows you to search for posts based on their location.', 'elasticpress-labs' ) . '</p>'; |
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.
$this->summary = '<p>' . __( 'Geo Location feature allows you to search for posts based on their location.', 'elasticpress-labs' ) . '</p>'; | |
$this->summary = '<p>' . __( 'Allow users to search for posts based on their location.', 'elasticpress-labs' ) . '</p>'; |
'has_map_key' => ! empty( $google_maps_api_key ), | ||
'is_external_meta' => has_filter( 'ep_geo_location_pre_geo_points' ), |
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.
As these will be used in JS, can we use camelCase here, please?
* @return array Mapping. | ||
*/ | ||
public function add_mapping( $mapping ): array { | ||
$mapping['mappings']['properties']['geo_point'] = [ |
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.
@burhandodhy this will only work on ES 7+. If geo_point is not available on ES 5.2 we should not allow people to activate it. If it is, we'll need something like
if ( version_compare( (string) Elasticsearch::factory()->get_elasticsearch_version(), '7.0', '<' ) ) {
$mapping_properties = &$mapping['mappings']['post']['properties'];
} else {
$mapping_properties = &$mapping['mappings']['properties'];
}
$mapping_properties['geo_point'] = [
* @return FeatureRequirementsStatus | ||
*/ | ||
public function requirements_status() { | ||
/** Features Class @var Features $features */ |
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.
Is the purpose of this line to help with autocomplete?
*/ | ||
protected function process_geo_distance_sort( $sort, $args ): array { | ||
foreach ( $sort as $key => &$sort_item ) { | ||
if ( isset( $sort_item['geo_distance'] ) ) { |
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.
Mind sharing the rationale behind this renaming?
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.
Overall it is looking really really good, @burhandodhy! We are almost there!
Description of the Change
This PR adds a new feature Geo Location
Closes #92
How to test the Change
Changelog Entry
Credits
Props @burhandodhy @felipeelia
Checklist: