Create module #10
Create module #10
Conversation
| } | ||
|
|
||
| // Added this function to change the output format to be more helpful | ||
| const diffStyles = (before, after) => { |
There was a problem hiding this comment.
Noting that this last function is the only change in this file (other than being moved). It formats the output we were getting to be more helpful.
| </script> | ||
| </html> | ||
| <script src="https://d3js.org/d3.v4.min.js"></script> | ||
| <script src="src/lib/diff.js"></script> |
There was a problem hiding this comment.
The only thing in this file that changed is the path on this script tag and removing sampleSet.js which I don't think we need. Prettier reformatted the file, but I'm happy to revert formatting if you prefer.
There was a problem hiding this comment.
I feel like we should load the built version of the tool (dist/main.js) instead, otherwise the formatting is great.
There was a problem hiding this comment.
This makes sense although I'm getting this error on trying to import from there:
Then if I use dist/module.js it only works if I specify the built function name: $cc04822511c4cad9$var$diffStylesSetStyle as opposed to diffStylesSetStyle
Should I go ahead with that ☝️ or do you think there's a better way?
We could also leave this script reference as-is and build the viewer into dist too with a parcel script: serve: parcel stamen-diff.html . This will build the viewer into our dist dir as well with module references (although these appear to be redundant).
There was a problem hiding this comment.
Ah okay, that's fair. Let's leave this be for now in favor of the follow-up issues.
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
left a comment
ebrelsford
left a comment
There was a problem hiding this comment.
This is a great start, thanks for putting it together! I have a small tweak around how we load the JS in the viewer, otherwise let's get this in place.
| </script> | ||
| </html> | ||
| <script src="https://d3js.org/d3.v4.min.js"></script> | ||
| <script src="src/lib/diff.js"></script> |
There was a problem hiding this comment.
I feel like we should load the built version of the tool (dist/main.js) instead, otherwise the formatting is great.
|
I'd like to propose these follow-up issues:
|
ebrelsford
left a comment
ebrelsford
left a comment
There was a problem hiding this comment.
Let's merge this and create follow-up issues.
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
setStyleproperties.To have parity with the existing tool, I also left the standalone
stamen-diff.htmlin 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 tomasterand 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.jsgets updated by an additional function appended todiff.jsfor the sake of simplicity/keeping this work scoped small (that function is now nameddiffStylesand the previous output comes fromdiffStylesSetStyle), 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.