FIX Layer with empty data #78
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/carto-frontend/web-sdk/cc9so7957 |
| const metadata = { geometryType, stats: fieldStats }; | ||
| if (stats.geometryType) { | ||
| geometryType = parseGeometryType(stats.geometryType); | ||
| } |
There was a problem hiding this comment.
Should we at least add a console.warn about empty source? Or maybe even have a source.isEmpty property based on this value?
| @@ -50,6 +50,11 @@ export function colorBinsStyle( | |||
| ) { | |||
| const evalFN = (layer: StyledLayer) => { | |||
| const meta = layer.source.getMetadata(); | |||
|
|
|||
| if (!meta.geometryType) { | |||
There was a problem hiding this comment.
I would better go after a layer.source.isEmpty prop, hiding this... what do you think?
|
|
||
| const metadata = { geometryType, stats: fieldStats }; | ||
| if (stats.geometryType) { |
There was a problem hiding this comment.
Maybe it easier to do something like:
if (!stats.geometryType) {
return { 'Point', stats: [] }
}
And even better, if you encapsulate the check !stats.geometryType in a method called hasData.
I would expect you don't need to check if geometryType exists and it is not going to fail for that reason in other places
There was a problem hiding this comment.
Same thoughts here around 'hasData' or 'isEmpty' method
|
Not sure if I understood your suggestions: I've added a method |
| @@ -214,6 +215,10 @@ export class SQLSource extends Source { | |||
| } | |||
| } | |||
|
|
|||
| public isEmpty() { | |||
There was a problem hiding this comment.
Nice, I think this is useful. Although I see the default method in the generic Source, I'm missing it in the GeoJSON one. I think we should try to keep the Sources as aligned as possible. What do you think? In that case I would return something like "features.length === 0"…
src/lib/viz/source/Source.ts
Outdated
| @@ -60,6 +60,11 @@ export abstract class Source extends WithEvents { | |||
| this.registerAvailableEvents(['filterChange']); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line class-methods-use-this | |||
| public isEmpty() { | |||
There was a problem hiding this comment.
Why not an abstract method? I find strange to see an implementation in the interface...
There was a problem hiding this comment.
I wanted to add a default implementation to avoid having to add the method in every source. But I reconsidered your suggestion and I agree that every source should implement its own method.
|
@manmorjim Some small comments / suggestions. Feel free to merge after considering them |
CH: https://app.clubhouse.io/cartoteam/story/90394