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

Update style to latest #268

Closed
wants to merge 2 commits into from
Closed

Conversation

danrademacher
Copy link
Member

Pulled in the state of the map style as of this commit, OpenHistoricalMap/map-styles@5f0c7fd

This is part of diagnosing OpenHistoricalMap/issues#956 (comment)

Pulled in the state of the map style as of this commit, OpenHistoricalMap/map-styles@5f0c7fd
Copy link

1 Warning
⚠️ Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?

Generated by 🚫 Danger

@danrademacher
Copy link
Member Author

@erictheise this PR has the latest map style in it, which is the one to use as the base for expressions-dependent multilingual labels

This is a locally testable fix for OpenHistoricalMap/issues#956, while we await the under-progress work to implement map style deployment via NPM.
@danrademacher danrademacher marked this pull request as ready for review February 6, 2025 20:02
@danrademacher
Copy link
Member Author

@erictheise this PR patches in a fixed version of the latest Map Style where @tsinn resolved an issue that caused Ruben to roll back the map style.

The version of the style here is the same as the one at https://github.com/OpenHistoricalMap/map-styles/blob/staging/main/main.json, so if you are very close on implementing the NPM package method of deploying map styles, we could just ignore this PR. An NPM version of latest map-style would flow in and resolve this issue.

If NPM map style approach remains blocked by the RAILS_MASTER_Key, we could test and merge this PR and then I'd also change the map-style commit hash in ohm-deploy to get back on track with the map style.

We could plan that for next week, giving you some time to get the NPM map-styles approach resolved

@erictheise
Copy link
Member

erictheise commented Feb 6, 2025

If NPM map style approach remains blocked by the RAILS_MASTER_Key, we could test and merge this PR and then I'd also change the map-style commit hash in ohm-deploy to get back on track with the map style.

We could plan that for next week, giving you some time to get the NPM map-styles approach resolved

@danrademacher, today's RAILS_MASTER_KEY issue was blocked a very simple deploy to staging–some translations and minor changes to JavaScript, JSON, and YML–and not the npm map-styles module work.

@1ec5 raised some concerns about legacy and other map projects that might be affected by changes in filenames and urls; they seem well-catalogued from an exercise last summer. The redirect solution makes sense to me but that would best be carried out by @Rub21 at the application server level (apache or nginx). I would like @tsinn and/or @vknoppkewetzel to weigh in on the impact of the proposed changes to their workflow.

I categorized the PRs as "draft" so that they wouldn't get merged until there was some discussion but the code works as is. I should rebase it against yesterday's c5cbd54b85fc6788159fd90f6c24e9a9a109ab26 but I'd like to see it on staging after OpenHistoricalMap/ohm-deploy#472 goes out, and shoot for getting it into production early next week. It is one of those deploys that I'd like to carve out some time for in case we find it needs a hotfix.

@danrademacher
Copy link
Member Author

Ok, based on your last comment, I'm closing this PR in favor of OpenHistoricalMap/map-styles#32 (comment)

And I'm going to see about having Vanessa and Tim join our dev meeting on Tuesday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants