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

point iD to ohm-editor-layer-index instead of the original osm index #73

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

andrewharvey
Copy link

@andrewharvey andrewharvey commented Jul 10, 2021

I'm not sure exactly how this fork is managed and kept up to date etc, so please let me know if I've done this wrong.

Some for the base branch, I'm not sure if this should target staging or not?

In the editor-layer-index fork I've temporarily disabled translations until we can set it up, looks like this switch might not work until that's done.

@danrademacher
Copy link
Member

On attempting to install and run this branch locally, I get:
npm ERR! premature close

Looking in the log, the source of the error looks like this:
17 silly fetchPackageMetaData error for ohm-editor-layer-index@github:openhistoricalmap/ohm-editor-layer-index#dist premature close

That occurs regardless of Node version as far as I can tell. I treid v10 (iD's stated min), v13, and v14.17.3.

I suspect we need to publish a node module out of the layer index repo, but were you able to install and run iD locally using the code in this PR?

andrewharvey added a commit to OpenHistoricalMap/ohm-editor-layer-index that referenced this pull request Jul 13, 2021
@andrewharvey
Copy link
Author

@danrademacher Thanks for reviewing, I should have tested this more myself first.

I believe I've fixed the issue now.

npm is trying to fetch the package directly from GitHub, but that's expected, it's how iD does it since the package is not published to the npm repository.

The issue was I had created a separate dist branch to publish the generated outputs which is what we reference here, but it was missing the package.json file, which I've now added.

I'm still now sure this will work without either making some more code changes or setting up our own OHM project on Transifex for translation of source names. I originally disabled that part because we don't yet have a Transifex project, but it looks like the iD integration here is looking for the translation strings.

@danrademacher
Copy link
Member

Ah, I see, the error I am getting is:
Error: ENOENT: no such file or directory, open 'node_modules/ohm-editor-layer-index/i18n/en.yaml'

In the name of just getting this running, I assume it would be possible to manually recreate a file like this at the above location
https://github.com/osmlab/editor-layer-index/blob/gh-pages/i18n/en.yaml

That would kick the can down the road as to how the translations could be made easily, but they would be possible by hand editing versions of that file for each language. Looks like we'd have to harvest the IDs from all the source GeoJSONs to get the correct list of IDs to then add translations for each one.

Hmm, possible but not easy to maintain

@andrewharvey
Copy link
Author

I anticipated we'd see that error, actually you're right, in the short term without any actual translations needed we don't need to setup Transifex.

I've re-enabled the code which generates that missing en file. So it should work now 🤞

@gregallensworth
Copy link

This pulled, built, and ran for me A-OK. 👍

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.

3 participants