Skip to content

Sequence mode - #70

Open
matthewjmckinley wants to merge 3 commits into
masterfrom
sequence-mode
Open

Sequence mode#70
matthewjmckinley wants to merge 3 commits into
masterfrom
sequence-mode

Conversation

@matthewjmckinley

Copy link
Copy Markdown

Add option to 'step' through multiple locations sequentially in Geolocation Map block within Exhibit Builder pages

@zerocrates zerocrates left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty good, and small which is always a positive.

My comments are mostly geared toward how you've followed the patterns that were already there in Geolocation, and it's probably time that we move away from them.

<?php echo $this->geolocationMapOptions(); ?>);
geolocation_map.initMap();
var map_locations = <?php echo json_encode($locations); ?>;
<?php if ($sequenceMode): ?>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we'd rather not do what you're doing there of having a php-level if add/remove a bunch of inline js... better to indicate sequence-ness with a data attr or class or something and switch on that inside the JS.

}
var currentIndex = 0;
var total = layers.length;
var prevBtn = jQuery('#<?php echo $divId; ?>-prev');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to the above, we'd probably rather avoid this interpolation inside the script.

The old code does it too of course... this is probably a good opportunity to take that out also: I think let's just put all that stuff that's getting interpolated into the OmekaMap constructor into data attrs on the map div. Of course the ID is already taken care of in the id attr.

Then we should also be able to make this code just a JS file: on doc ready loop over .exhibit-geolocation-map and do this startup for each.

function goToStep(index) {
currentIndex = index;
var layer = layers[currentIndex];
var geometry = JSON.parse(map_locations[currentIndex].geometry_json);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would think you can avoid reparsing the geo json for each point when you go to it... it looks like you're only using it to check if it's a Point or something more complex. Points are L.marker so you should be able to do layer instanceof L.marker to decide if you're looking at a point or not.

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.

2 participants