Major marker support #16
Conversation
|
I think you should include @alonsogarciapablo in this PR so he gets familiar with tangram |
|
Done! |
|
also fixing CI would be nice |
|
I promise to fix it in my last commit :) |
|
Fixed as I promised @javisantana :) |
| @@ -49,32 +50,35 @@ | |||
| "license": "MIT", | |||
| "dependencies": { | |||
| "babel-runtime": "6.11.6", | |||
| "carto": "CartoDB/carto#master" | |||
| "carto": "CartoDB/carto#master", | |||
There was a problem hiding this comment.
be careful when pointing to a non-tag because you could be generating a version that does not work without noticing
There was a problem hiding this comment.
I'll change it. Thanks!
| export default BaseHelpers; | ||
|
|
||
| const OPACITY = { | ||
| line: 'stroke-opacity', |
There was a problem hiding this comment.
2 spaces identation is what we usually use
There was a problem hiding this comment.
ups... I'm changing from one project to other and I forgot to change the settings, we need a editorconfig.
| @@ -0,0 +1,100 @@ | |||
| /* | |||
There was a problem hiding this comment.
do we need this banner for each file? 
There was a problem hiding this comment.
hahaha, just to make it beautiful :)
| } | ||
| }; | ||
|
|
||
| // NOTE: this no makes sense actually. |
There was a problem hiding this comment.
But I'm in the middle of something... It is gonna be necessary when we add some more features. It's just to remember.
There was a problem hiding this comment.
It makes sense so far. Some suggestions:
- I'd try to clarify variable/module names a bit, and be a bit more explicit. We're working with objects from different domains (TurboCarto, CartoCSS, Tangram, CARTO, ...). I'd try to be a bit more explicit with names in general.
- I'd start adding some tests for
src/basic/points.js,src/basic/lines.js,src/basic/polygons.js. Those could help understand the project a bit better.
That's it! Great job so far @donflopez!
| INTERNAL LINE FUNCTIONS | ||
| */ | ||
|
|
||
| const getLineAlpha = function(c3ss) { |
There was a problem hiding this comment.
What does c3ss mean? Could we find a more explicit variable name?
There was a problem hiding this comment.
compiled carto css => c3ss
It is the most repeated variable in the project, so it could be an important effort to write it as compiledCartoCSS 😓
There was a problem hiding this comment.
It is the most repeated variable in the project
Then it's even more important to find a name that anyone will understand 😝
There was a problem hiding this comment.
Don't you think that a good documentation could be better? Too much words in the code could make it less readable and more dirty.
|
|
||
| return draw; | ||
| }; | ||
|
|
||
| const extractFeatures = function (ccss) { | ||
| let layers = CCSS.render(ccss).getLayers(), |
There was a problem hiding this comment.
I find CCSS a bit confusing here... Why not name the variable CartoCSSRenderer?
|
|
||
| import BH from './helpers'; | ||
| import Utils from '../utils/utils'; | ||
| import TR from '../utils/reference'; |
There was a problem hiding this comment.
I personally find TR, BH... and this kind of names a bit confusing. IMO everything would me more clear with names like TangramReference, SomethingHelper, etc.
| let ly = layers[i].shader; | ||
|
|
||
| draws = getSymbolizers(ly); | ||
| let ly = layers[i].shader; |
There was a problem hiding this comment.
Perhaps we can infer the type of geometry from the shader?
| @@ -0,0 +1,33 @@ | |||
| import Utils from '../utils/utils'; | |||
There was a problem hiding this comment.
helpers.js... is very generic too... something-helper.js would help us understand that all functions here are related to "something"....
With this PR we'll have major support for markers.
Add marker rectangle type.to be added in other PR tangram doesn't support it yetcc/ @rochoa, @javisantana & @alonsogarciapablo