Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expose option to decode domain values #450

Merged
merged 3 commits into from
Nov 27, 2018
Merged

expose option to decode domain values #450

merged 3 commits into from
Nov 27, 2018

Conversation

jgravois
Copy link
Contributor

@jgravois jgravois commented Nov 16, 2018 •
edited
Loading

relies on Esri/arcgis-rest-js#392 and Esri/arcgis-rest-js#394

  • bar chart
    chart w/ more than 1 dataset (this can be test instead of an example if that's easier)
    pie chart? (maybe)
    line/area (pretty much the same as bar)

tomwayson
tomwayson approved these changes Nov 16, 2018
View reviewed changes
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good so far!

docs/index.html Outdated
<!-- load the arcgis-rest-js scripts -->
<script src="https://unpkg.com/@esri/arcgis-rest-request@1.7.1/dist/umd/request.umd.min.js"></script>
<script src="https://unpkg.com/@esri/arcgis-rest-feature-service@1.7.1/dist/umd/feature-service.umd.min.js"></script>
<!-- you should have to load arcgis-rest-js scripts, but right now cedar bundles them! -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been taken care of in https://github.com/Esri/cedar/pull/421/files - I must have missed something, or done something wrong...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll tackle this in #391

packages/cedar/src/dataset.ts Outdated
@@ -1,16 +1,17 @@
import { IFeature, IFeatureSet } from '@esri/arcgis-rest-common-types'
import { IField as IRestField, IFeature, IFeatureSet } from '@esri/arcgis-rest-common-types'

export interface IField {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: deprecate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately not. your IField interface is its own beast.

packages/cedar/src/dataset.ts
@@ -66,7 +67,7 @@ function getDatasetData(dataset, datasetsData, name?) {
// return only the attributes for each feature
function flattenData(data) {
const features = getFeatures(data)
if ((features[0] as IFeature).attributes) {
if (features.length > 0 && (features[0] as IFeature).attributes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

packages/cedar/src/query/query.ts Outdated
names.push({
// TODO: make name required on datasets, or required if > 1 dataset?
name: dataset.name || `dataset${i}`,
url: dataset.url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can revert the names array back on an array of strings, right?

packages/cedar/src/query/query.ts Outdated
}
})
}
return Promise.all(requests)
.then((responses) => {
// turn the array of responses into a hash keyed off the dataset names
const responseHash = responses.reduce((hash, response, i) => {
hash[names[i]] = responses[i]
return hash
hash[names[i].name] = response;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops!

packages/cedar/src/query/query.ts Outdated
}, {})
return Promise.resolve(responseHash)
return Promise.resolve(responseHash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZOMG tslint, what is it you would say you do around here?

tomwayson
tomwayson reviewed Nov 18, 2018
View reviewed changes
.travis.yml Show resolved Hide resolved
@tomwayson
Copy link
Member

Assuming we decide to add domains as described in Esri/arcgis-rest-js#394 (comment), I think we'll want to use that instead of fields here.

jgravois
jgravois commented Nov 26, 2018
View reviewed changes
Copy link
Contributor Author

@jgravois jgravois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a few edits to address feedback after confirming locally that i could pass along domains to the bar-grouped.json sample too.

docs/index.html Outdated
<!-- load the arcgis-rest-js scripts -->
<script src="https://unpkg.com/@esri/arcgis-rest-request@1.7.1/dist/umd/request.umd.min.js"></script>
<script src="https://unpkg.com/@esri/arcgis-rest-feature-service@1.7.1/dist/umd/feature-service.umd.min.js"></script>
<!-- you should have to load arcgis-rest-js scripts, but right now cedar bundles them! -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll tackle this in #391

@jgravois
Copy link
Contributor Author

travis is asleep right now, but running npm test locally you'll see that test coverage is compromised.

All files           |    98.73 |    96.92 |      100 |    98.71 |                   |
 src                |      100 |      100 |      100 |      100 |                   |
  Chart.ts          |      100 |      100 |      100 |      100 |                   |
  config.ts         |      100 |      100 |      100 |      100 |                   |
  dataset.ts        |      100 |      100 |      100 |      100 |                   |
  index.ts          |      100 |      100 |      100 |      100 |                   |
 src/query          |    95.12 |    90.91 |      100 |       95 |                   |
  query.ts          |    91.67 |    85.71 |      100 |    91.67 |             38,44 |
  url.ts            |      100 |      100 |      100 |      100 |                   |

i tried to add a new test with a new definition that includes a dataset with domains and failed definitively.

@jgravois jgravois mentioned this pull request Nov 27, 2018
Merged
@jgravois jgravois reopened this Nov 27, 2018
@tomwayson tomwayson merged commit 08ab30f into master Nov 27, 2018
@tomwayson tomwayson deleted the feat/cvds branch November 27, 2018 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomwayson tomwayson tomwayson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jgravois @tomwayson