Mobx SOS #4167
Mobx SOS #4167
Conversation
This currently implements only mode#1 from `master` branch.
|
Test catalog file - https://gist.github.com/na9da/12b0ea6ca0a4ef46c83019b77f8fa080 |
|
|
I fix this in #4219 : |
|
Reviewing now. |
| @@ -29,7 +29,7 @@ module.exports = function(hot, dev) { | |||
| { | |||
| // Don't let jasmine-ajax detect require and import jasmine-core, because we bring | |||
| // in Jasmine via a script tag instead. | |||
| test: require.resolve('terriajs-jasmine-ajax'), | |||
| test: require.resolve('jasmine-ajax'), | |||
There was a problem hiding this comment.
Hey @na9da. Can you explain changing terriajs-jasmine-ajax to jasmine-ajax?
There was a problem hiding this comment.
Is it one/a mix of:
- Everything we needed in
jasmine-ajaxgot merged upstream - We no longer need the things we did need to make the custom package for because we're not using Browserify any more
There was a problem hiding this comment.
My initial reasoning was that our fork is 4 years old and hey this change didn't break any tests so lets ship it 😄 I was also looking to use a feature that didn't exist in our fork.
Then I found this PR from Kevin. I think the code he is trying to fix is no longer present in upstream. We are also not using browserify any more. So I guess it ticks both of your rationale.
| checkAllPropertyKeys(node.attribs, this.attributes); | ||
|
|
||
| const catalogItem: SensorObservationServiceCatalogItem = <any>( | ||
| context.catalogItem |
There was a problem hiding this comment.
Can we be certain this is a SOS catalog item? If not it might be better to throw a TerriaError if it's not SOS chart compatible.
There was a problem hiding this comment.
Fixed by throwing DeveloperError. I think that's more appropriate here.
| @@ -19,6 +20,7 @@ import CustomComponent from "./CustomComponent"; | |||
| */ | |||
| const registerCustomComponentTypes = function(terria) { | |||
| CustomComponent.register(new ChartCustomComponent()); | |||
| CustomComponent.register(new SOSChartCustomComponent()); | |||
There was a problem hiding this comment.
Why do we need a custom chart component for SOS?
There was a problem hiding this comment.
The current chart component is dependent on CsvCatalogItem. I have created a ticket for writing a more generic chart component #4264.
What this PR does
Partially Fixes #3896
Ports SensorObservationCatalogItem:
Not handled in this PR:
ChartCustomComponentto be more generic so that a single '' component can handle all chart components instead of introducing new ones like<air-quality-chart>or<sos-chart>Checklist