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

Refactor Google Maps Plugin for API key #73

Merged
merged 25 commits into from
Nov 26, 2019

Conversation

titaniumbones
Copy link

Hi Menismu, thanks for all your work on this project. I am hoping to use it in a student assignment a couple of weeks from now so I'm trying to get a few things working before then :-) . I've started work on #68 in this branch -- just in the early stages, but submitting this PR early just to let you know I'm working on it.

I think there will be some issues around testing, since the API key probably shouln't live in-repo.

Here's what I've done so far (very minor):

  • remove deprecated "sensor=false" from gmaps API call
  • add &key=$apiKey to API call
  • add options.apiKey to plugin definition

TODO: find some way to add tests

- remove deprecated "sensor=false" from gmaps API call
- add `&key=$apiKey` to API call
- add options.apiKey to plugin definition

TODO: find some way to add tests
temporary workaround for mensimu/popcorn-js#74.
titaniumbones added a commit to DigitalHistory/advanced-topics that referenced this pull request Mar 11, 2019
@titaniumbones titaniumbones changed the title DO NOT MERGE: begin refactoring Google Maps Plugin for API key Refactor Google Maps Plugin for API key Mar 11, 2019
@titaniumbones
Copy link
Author

Current version of this PR now works if API key is authorized for the calling URL. Tests have not been updated; see discussion in #68 for some issues on that topic.

@titaniumbones
Copy link
Author

@menismu have you had a chance to look at any of these PR's? Looks like you've been off of GH for a month or so -- looking forward to discussing the future of Popcorn when you get back.

Included jar was from 2110 (!) and coul not digest modern js.

This is still a pretty outdated way to uglify code, though.
- [ ] show HTML instead of plain text
- [ ] remove confusing/extraneous text from display
- [ ] remove images
- [ ] replace `numberofwords` option with `paragraphs`
- [ ] create separate wrapper div w/ unique id for each plugin
instance
- [ ] use fetch API instead of Popcorn.getScript for Wikipedia API
call.
- google feed serice has been disocntinued. Move this plugin into
`unsupported` until it can be rewritten to call the RSS feed
directly (!) or use someo ther service
- `mediaspawner` is currently broken and I dont' have time to fix it.
Move to unsupported until someone can take that over
- `openmap` also fails tests, and the popularity of openlayers has
diminished. Deprecting for now.
- [ ] retain `http[s]:` at front of URL, or loading from `file://`
URL's fails
- [ ] don't hard-code iframe height; instead addd a class that can be
addressed by CSS in user code
Add a markdown plugin based on `popcorn.mustache.js`.

Needs better tests, but so do almost all the plugins :-(.
- remove commented-out code
- improve docstring
- protect against errors
- adjust tests so plugin now passes
should pass all tests now
Adds a `leaflet` plugin, modelled on the existing OpenMaps plugin,
but adidng a `fly` parameter that pans the map form initial location
to a chosen endpoint.
use Array.prototype.forEach instead of bespoke Poprorn.forEach, which
was anyway probably improperly ocnstructed.
for ocmpatibility w/ other plugins.
The `image` plugin was initially designed largely for overlays on
playing videos, and makes a number of aggressive and outdated CSS
assumptions that are unhelpful in the context of other uses. Rather
than refactor & break `image`, this new plugin uses the `figure` tag
as a container for plugin instances & applies only minimal styling,
allowing the user to control display properties by addressing the
`figure-plugin` class or individual plugin instance ID.
add container id and frame to the following plugins:

- leaflet
- markdown
- text
- webpage
- Wikipedia

Also move `.style.display = "none"` up in a couple of places to avoid
brief flashes of content on screen.
Previous iteration had an odd level of specificity, and created a
single div in any given target for the wordriver.  By adding an ID, we
can create one or multiple wordriver instantiations in the target.

I *hope* this works OK. I'm a little worried that this new parent will take up
space even when the contents are hidden.
- add ID property to manifest & examples
- move most setup code into `_setup`
- set `id` on **container**, and add classes to both container and
content div
- replace hard-coded CSS attributes, breaks, and spans (!) with standard HTML
elements (h3, p)
@menismu menismu merged commit 2c729c5 into menismu:master Nov 26, 2019
titaniumbones added a commit to titaniumbones/UTSC-map-exercise that referenced this pull request Feb 18, 2021
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