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

Initial commits of blacklight-maps gem #1

Merged
merged 3 commits into from Mar 12, 2014
Merged

Initial commits of blacklight-maps gem #1

merged 3 commits into from Mar 12, 2014

Conversation

mejackreed
Copy link
Contributor

Would appreciate review by @jcoyne, @cbeer, and @jkeck

<!-- map goes here -->
<%= show_map_div() %>
<div id="leaflet-sidebar"></div>
<%= javascript_tag "var docs = #{@response.docs.to_json};" %>
Copy link

Choose a reason for hiding this comment

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

Perhaps we can come up with a more lightweight serialization of the document array since we already know the fields that we'll need through configuration (e.g. title, latlong, placename, etc). This could balloon up in size for large responses of complex documents.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully that's what @jcoyne was looking at in https://github.com/sul-dlss/spotlight/tree/list_of_points, and I think/hope we can get the detailed results view from an AJAX request to catalog#index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to only include needed mapping fields. https://github.com/sul-dlss/blacklight-maps/blob/setup/app/helpers/blacklight_maps_helper.rb

For the BL default seed data (30 records) this reduces the size of the JS object (stringified) from 17,028 bytes to 3,160 bytes (82% reduction)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9e29b799ed92d3456d3d979b4790e0cdfede1d19 on setup into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ead1a034c823bba42a2494cf1c005edebbe1d8a0 on setup into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ead1a034c823bba42a2494cf1c005edebbe1d8a0 on setup into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ead1a034c823bba42a2494cf1c005edebbe1d8a0 on setup into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ead1a034c823bba42a2494cf1c005edebbe1d8a0 on setup into * on master*.

@cbeer
Copy link
Member

cbeer commented Mar 12, 2014

+1. I think it's good enough to ship and then clean up. I'll leave some inline comments for discussion, but I think we can address them in later PRs.


gem 'simplecov', require: false
gem 'coveralls', require: false

gem 'engine_cart', git: 'https://github.com/cbeer/engine_cart'
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this gem engine_cart can get dropped. The gemspec should be providing it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbeer There was some reason I left it in there and now I can't remember. I'll look into it.

@cbeer
Copy link
Member

cbeer commented Mar 12, 2014

@jkeck this (still) has a 👍 from me. If you don't see any blockers, let's :shipit:.

mejackreed added a commit that referenced this pull request Mar 12, 2014
Initial commits of blacklight-maps gem 0.0.1
@mejackreed mejackreed merged commit af2a381 into master Mar 12, 2014
@cbeer cbeer deleted the setup branch March 12, 2014 18:18
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.

None yet

4 participants