Add custom color ramp graphic for interpolated color ramps - #236
Conversation
….stanford.edu into feature/62-add-color-ramp
megannissel
left a comment
megannissel
left a comment
There was a problem hiding this comment.
I'm still not 100% sure I fully understand how the custom colormap stuff with TiTiler works (in practice, once the custom logic is in place), but these changes look reasonable. Just a couple of notes (including a build failure we ought to address) for your consideration.
| if isinstance(cmap, Sequence): | ||
| return [(k, numpy.array(v).tolist()) for (k, v) in cmap] | ||
| else: | ||
| return {k: numpy.array(v).tolist() for k, v in cmap.items()} |
There was a problem hiding this comment.
I don't think this code block is accessible due to the return on line 136
There was a problem hiding this comment.
Yeah, I think that just ended up being some copy-pasta from the function I based it off of. Removed in 48bd735
| WORKDIR /tmp/mapbox-gl-legend | ||
| RUN apk add git && \ | ||
| git clone https://github.com/natcap/mapbox-gl-legend.git . && \ | ||
| git checkout 924289931c9c502055900fc16d767313886ba9b0 && \ |
There was a problem hiding this comment.
It looks like the build failure is related to this ref update. We ought to figure out what's going on there before merging this!
There was a problem hiding this comment.
Turns out typescript was being extra picky that I had a module-level variable defined but never used. Patched the build in 2b9631e
| @@ -1,3 +1,15 @@ | |||
|
|
|||
There was a problem hiding this comment.
Should we replicate all of the changes in this file in ckan/Dockerfile.dev as well, so that our local dev cluster mirrors what's in production?
megannissel
left a comment
megannissel
left a comment
There was a problem hiding this comment.
I think there's one more line in the Dockerfile that needs to be added to Dockerfile.dev for parity.
Also, just to double-check for merging and deploying: I believe it'll be totally fine that merging this will trigger a tileserver deploy, since none of the changes to the tileserver rely on changes to CKAN. And once the new tileserver deploy has happened, we can go ahead and re-deploy CKAN (though we ought to deploy to staging first just to confirm everything's working as intended).
| # Copy custom initialization scripts | ||
| COPY ./ckan/docker-entrypoint.d/* /docker-entrypoint.d/ | ||
|
|
||
| COPY --from=mapboxgl-legend-builder /tmp/mapbox-gl-legend/dist/cdn/mapbox-gl-legend.js /srv/app/src/ckanext-mappreview/ckanext/mappreview/assets/js/vendor/mapbox-gl-legend.js |
There was a problem hiding this comment.
Looks like this didn't make it into Dockerfile.dev
There was a problem hiding this comment.
Ugh thanks for catching this. Patched in 49fc454
megannissel
left a comment
megannissel
left a comment
There was a problem hiding this comment.
Looks good, thanks @phargogh! As discussed on Slack, we'll merge this to trigger the tileserver deploy, but will hold off on deploying CKAN for now.
2 participants
This PR adds an endpoint to titiler that generates a graphic representing the color ramp (#62 ) based on a couple parameters.
Most of the lines changed are just my import linter updating imports, but the endpoint added is nontrivial anyways.
This PR is the first of several, and merging some version of this functionality in and deploying the new titiler is a key part of getting the rest of the color ramp functionality working in prod.