Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Completely rewrite pelias compare tool in vue #21

Closed
wants to merge 25 commits into from
Closed

Completely rewrite pelias compare tool in vue #21

wants to merge 25 commits into from

Conversation

blackmad
Copy link

@blackmad blackmad commented Mar 3, 2020

Benefits of this rewrite

  • not in angular1
  • interactive form for lots of pelias options, including autocomplete, debug and focus
  • interactive map to select focus point
  • linkified json to WOE entities and openaddress definition files
  • clicking on a result zooms the map to it
  • doesn't require checking in bower_components
  • compiles down to a couple of files that I intend on figuring out a good way to host inside of the pelias api server for quick on-shard debugging

Here's a link to the compiled output

http://dump.blackmad.com/pelias-compare2/dist/#/v1/search?text=london&debug=0

@orangejulius
Copy link
Contributor

orangejulius commented Mar 4, 2020 •
edited
Loading

This looks pretty awesome to me. Lots of great functionality we've wanted for a long time (check out #9 from 2015).

Since we've been adding demos/interactive interfaces to most of our services, one option would actually be to move this code into https://github.com/pelias/api directly. Then we can archive this repo and move on.

The main thing I'd like to make sure we achieve there is good developer ergonomics. Which basically means people doing general API development don't have to run npm install for the compare app unless they really want to. We definitely don't want to add all the deps for the compare app to the root package.json in the API. We already discussed some of the details of how we might achieve this when going over the Dockerfile stuff.

Other than that, I think this is basically good to go. I noticed some checkboxes are out of alignment (at least for me, Firefox on Linux), but that's it. It's really nice!
Screenshot_2020-03-03_18-30-30

@blackmad
Copy link
Author

blackmad commented Mar 4, 2020 via email

@orangejulius
Copy link
Contributor

Oh, I thought of another way we could manage things. This repository can publish an NPM module, which would contain only the compiled output for the compare app, and not the entire vue toolchain. Then we can either run that NPM module independently, to keep the current compare app on GitHub pages, or we can include it as a regular dependency in the API.

We already use NPM modules for Pelias dependencies so this would fit well with existing patterns. Let me know if you think that works and we can set it up together.

@blackmad
Copy link
Author

blackmad commented Mar 4, 2020 via email

@orangejulius
Copy link
Contributor

Yup, I would imagine it would work well with some combination of npm scripts (maybe prepublish?) and the files property in package.json to restrict what is part of the published module.

@blackmad
Copy link
Author

blackmad commented Mar 4, 2020

well ... that was easier than expected

this is the change to serve it from api/
https://github.com/blackmad/api/commit/3a9fb12489c00d02d561eed2ad52fa47889d394b

it's a 1.6mb zipped / 6mb unzipped dependency. I'm okay with always including it, in a separate PR we can talk about that.

For this repo/PR

  • still debugging the option display thing - I see it in firefox mac too.
  • what else do you need to feel comfortable merging this?
  • what would you like to do about gh-pages? I don't love the idea of checking the 6mb compiled docs/ into github just so it can be served from gh-pages, but that's a pretty easy solution
  • is it worth renaming the existing repo to compare-old, and having this be a fresh repo, so when cloning, people don't need to download all the old checked in bower components?

@orangejulius
Copy link
Contributor

Cool, glad it was easy to set up the NPM module thing. Can you add the pelias NPM user as an admin on the module?

Is it possible at all to have a build step with GitHub pages? I haven't gotten too in depth with it in a long time.

As for what to do with this PR, I think we can move the current master branch to old, and then force push a modified version of this branch with a separate commit history to master. That will keep the size of the clone lower, right?

@blackmad
minor bump
502c04c 
- fix options column display in firefox
- explicit .nojekyll for github pages
- don't auto-submit on autocomplete
@blackmad
Copy link
Author

blackmad commented Mar 4, 2020

re: display issue - fixed
re: npm - invited pelias
re: build step - no, github is jekyll or static files only
re: clone size - ... I have no idea. going to read up a bit because I guess I don't understand git

@orangejulius
Copy link
Contributor

orangejulius commented Mar 5, 2020 •
edited
Loading

Awesome.

Peter and I thought of some more things to take care of before merging this.

The most important is that we ensure existing compare app URLs don't break. We probably have thousands of them in issues across the project and the internet at large, many of them come with screenshots of current results, so we might actually stand a chance at being able to do this.

More specifically, Pelias supports several endpoints besides search and autocomplete. While we don't need to have specific handling for them, they should at least work by manually typing in a query. Again, the massive set of existing links could be useful here.

Here's a rough list of the most important things:

  • Reverse requests have no text parameter, only point.lat and point.lon
  • Structured geocoding also has no text param, but a plethora of other, admin area specific parameters
  • The Place endpoint only takes an ids parameter
  • In general, the most important parameters to search and autocomplete are sources, layers, and the focus point params focus.point.lat and focus.point.lon

@blackmad
Copy link
Author

blackmad commented Mar 5, 2020 via email

@missinglink
Copy link
Contributor

Is it possible to maintain the existing copy->paste behaviour?

The current CSS is a bit of a hackjob so copy->paste works nicely and it still renders ok.

1) Springfield, MO, USA
 2) Springfield, MA, USA
 3) Springfield, OR, USA
 4) Springfield, OH, USA
 5) Springfield, VA, USA
 6) Springfield, TN, USA
 7) Springfield, FL, USA
 8) Springfield Township, MO, USA
 9) Springfield Township, OH, USA
10) Springfield Township, OH, USA

London, England, United Kingdom
 City of London, London, England, United Kingdom
 London, ON, Canada
 London, OH, USA
 London, South Africa
 London, OH, USA
 London, OR, USA
 London, Sierra Leone
 London, Guadeloupe
 London, South Africa

@missinglink
Copy link
Contributor

Possibly off-topic considering the discussion of keeping the existing urls still working...

Considering an API request as such:
GET /v1/autocomplete?text=foo

It would be ideal if you could send the exact same request but to another domain, say ui.geocode.earth instead of api.geocode.earth and it displayed a UI such as this instead of serving JSON.

The nice thing about that approach is that the user can literally just change the hostname to switch between a GUI and the JSON API.

@blackmad
Copy link
Author

blackmad commented Mar 5, 2020 via email

@missinglink
Copy link
Contributor

missinglink commented Mar 6, 2020 •
edited
Loading

Yes that depends if you're aiming for a framework refactor with backwards compatibility (to run on the same domain) or creating a replacement that would have a different url structure which we could forward rewritten requests to?

I'm happy with either, but considering you've already spent more time on it than we did on the angular version, a new domain would allow you more flexibility to be creative.

We would always have the opportunity to forward requests off the GitHub 'compare' url to another location once it was stable, thereby making it the default UI.

Just a thought, maybe it's easier to chat on a video call about this?

I don't want to be a blocker, just thinking what a v2.0 would look like rather than a v1.1

@blackmad
Copy link
Author

blackmad commented Mar 6, 2020 via email

@blackmad
peter's requests
2b19fd5 
- support all five major pelias api endpoints + customize form for them
- decode uri-encoded location hashes
- pelias favicon
- hack list css to make copy paste better
- show focus point in modal
- start of work on SPA version that uses url routing not hash
@blackmad
Copy link
Author

blackmad commented Mar 6, 2020 via email

@blackmad
Copy link
Author

blackmad commented Mar 6, 2020 via email

@orangejulius
Copy link
Contributor

Awesome. It looks really good!

Peter, I love your suggestion of being able to have a domain that handles Pelias URLs, but with the compare app. I think since David has put the core of the new compare app in an NPM module, we can build that.

The main use of the NPM module will be to embed the compare app in the API. So it might be able to exist there. Alternatively we could create Yet Another Thing(tm) that also uses the compare NPM module to achieve that goal. I think we can follow up with it separately.

David if you think the URL handling is reasonably solid and compatible with whats out there, the NPM module is ready, and you are ready to hit merge (or rather figure out a more git repo friendly way to bring this in), let me know and we can coordinate on that. Thanks for all the work on this!

@blackmad
Copy link
Author

blackmad commented Mar 6, 2020 via email

@orangejulius
Copy link
Contributor

Okay great. We don't care about the history in this project, and we even already have a dedicated place for dead code to live. I'll move this entire repo to pelias-deprecated/compare-v1, create a new repo in its place, and you can create a PR there. :shipit: 🚢

@blackmad blackmad mentioned this pull request Mar 6, 2020
Merged
@blackmad blackmad closed this Oct 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blackmad @orangejulius @missinglink