search

stamen / mapbox-gl-style-diff

Other
2 stars 1 forks source link

Create module #10

Closed aparlato closed 2 years ago

aparlato commented 2 years ago

Closes #9

Creates a module from the diff tool following the format of https://github.com/stamen/mapbox-gl-style-format.

I updated the format of the output to look like the output we get in Maputnik which more clearly references a compare diff than the setStyle properties.

To have parity with the existing tool, I also left the standalone stamen-diff.html in place, and let it reference the old format letting it run without additional changes to keep this work scoped.

🚨 @ebrelsford also I see we're failing copying to S3, but I'm not sure why: fatal error: An error occurred (InvalidAccessKeyId) when calling the ListObjectsV2 operation: The AWS Access Key Id you provided does not exist in our records. Did our credentials change since this repo last had changes? I don't see anything here that should cause this and I pushed a test branch that's identical to master and that failed as well.

Followup

Right now, as mentioned, the standalone compare tool is using the old formatted output of the diff function. We should update it to use the latest format for clarity and further work.

Also, the format from diff.js gets updated by an additional function appended to diff.js for the sake of simplicity/keeping this work scoped small (that function is now named diffStyles and the previous output comes from diffStylesSetStyle), but ideally we'd remove this additional formatting function and just update the rest of the diffing function code to output to this format without further alteration.

Let me know if we want to handle either of these followups in this PR or if they make sense to address after as lower priority. Also, I feel like the standalone tool seems fine to leave in here, but we might consider another home for it on the consumer or at least excluding it from the module.

ebrelsford commented 2 years ago

I see we're failing copying to S3, but I'm not sure why [...] Did our credentials change since this repo last had changes?

Yes they did change! As mentioned in #11 I think we need to re-work how deploying this tool works, it's safe to ignore these checks for the moment.

ebrelsford commented 2 years ago

I'd like to propose these follow-up issues: