Add percentile extra params #230

fungjj92 · 2017-08-31T17:29:14Z

Overview

Adding the percentile params part of the extra params feature set.

Demo

Notes

I struggled with the copy.. Alternative ideas:

_ percentile of percentile low temperature
Low temperature projections at percentile: __

Eh none of them are very good. @jcahail @CloudNiner

Testing Instructions

The usual npm run serve

Closes #204

flibbertigibbet

Doesn't compile, due to apparent rebase against develop. See note on extending interface (must now list properties of parent's params).

flibbertigibbet · 2017-08-31T17:45:50Z

src/app/charts/chart.component.ts

        const thresholdParams = $event.data as ThresholdIndicatorQueryOpts;
        this.extraParams = thresholdParams;
        this.onExtraParamsChanged.emit(this.extraParams);
        this.updateChart(this.extraParams);
    }

+    public onPercentileSelected($event) {
+        const percentileParams = $event.data;
+        console.log(percentileParams);


Stray console log in here

flibbertigibbet · 2017-08-31T17:47:51Z

src/app/charts/extra-params-components/extra-params.constants.ts

+    'percentile_low_temperature',
+    'percentile_precipitation'
+];
+
 // TODO: concat additional extra parameter names to this array for #203, #204, and #205


Can remove #204 from TODO comment, which this closes

flibbertigibbet · 2017-08-31T17:49:05Z

src/app/charts/extra-params-components/percentile.component.html

+    <form [formGroup]="percentileForm" novalidate>
+        <div class="chart-options-body form-group">
+            <span class="optional-parameters-label">Show {{indicator.label | lowercase}} at percentile
+                <input type="number"


Could validate this to be in 0 to 100 range

flibbertigibbet · 2017-08-31T17:50:45Z

src/app/charts/extra-params-components/threshold.component.ts

- * Chart component
- * Container for each individual chart plus controls
+ * Threshold params component
+ * Multi-field form to allow user to specify threshold params


flibbertigibbet · 2017-08-31T17:51:46Z

src/app/models/percentile-indicator-query-opts.model.ts

+import { IndicatorQueryOpts } from './indicator-query-opts.model';
+
+export interface PercentileIndicatorQueryOpts extends IndicatorQueryOpts {
+    params: {


Will need to list all the properties of the extended params object here after having rebased against develop, due to library updates

flibbertigibbet · 2017-08-31T18:20:53Z

Still not compiling, but closer. Needs ClimateModel import.

fungjj92 · 2017-09-01T13:18:32Z

Technically this build succeeded, but a repeat build died due to a connection error

flibbertigibbet · 2017-09-01T14:07:39Z

You can restart the build manually in the Jenkins interface.

fungjj92 · 2017-09-01T16:50:37Z

Rebased against Andrew's merge

flibbertigibbet

Looks good! One more thing: could use some more error handling. It's still possible to type invalid text (such as a decimal), which will result in an infinitely loading chart.

CloudNiner

Glad the rebase wasn't too bad!

CloudNiner · 2017-09-01T17:56:52Z

src/app/charts/extra-params-components/extra-params.constants.ts

@@ -26,3 +31,12 @@ export function hasExtraParams(indicatorName: string): boolean {
    }
    return false;
 }
+
+export function isPercentileIndicator(indicatorName: string): boolean {
+    for (const percentileName of percentileIndicatorNames) {


Mostly trivial comment, but is there a reason these functions can't all be compressed down to the more commonly used:

export function isPercentileIndicator(indicatorName: string): boolean { return percentileIndicatorNames.indexOf(indicatorName) !== -1; }

flibbertigibbet · 2017-09-01T19:27:43Z

src/app/charts/extra-params-components/percentile.component.ts

+            const pctl = form.percentileCtl;
+            if (pctl > 100 || pctl < 1) { return; }
+            this.percentileParamSelected.emit({
+                'percentile': Math.round(pctl)


Using ng-pattern would be better, to enforce input restrictions rather than implicitly rounding what the user enters. ng-pattern="/^-?[0-9][^\.]*$/" should do the trick.

Although there are restrictions on the input now, there's no feedback to the user to indicate that the input is invalid; the chart simply fails to reload. It would be good to maybe highlight the field red or give some other error feedback.

fungjj92 · 2017-09-01T20:57:08Z

Peh. I managed to rebase onto the basetemp merge, but didn't succeed at better validation.
Was playing around with this, to no avail:
http://cuppalabs.github.io/tutorials/how-to-implement-angular2-form-validations/

flibbertigibbet

+1 on tests passing and an issue made for form validation feedback (if it doesn't go in this PR).

Looks like there's a comma missing in the login component test. Maybe we should just remove that test, since it seems to be a hassle to maintain. It is the only component test we have, though.

If the form validation is too pesky, maybe can just make an issue for form feedback.

CloudNiner

@flibbertigibbet since we've both approved this and @fungjj92 is out, can you create the referenced issue and get this merged?

flibbertigibbet · 2017-09-05T15:33:27Z

There is a small issue causing tests to fail, which could be fixed up by a subsequent rebase in #242.

So yes, I'll do that.

flibbertigibbet · 2017-09-05T15:35:31Z

Created #243

flibbertigibbet · 2017-09-05T15:40:26Z

Just rebased in a fixup to the test commit, will merge on test pass

flibbertigibbet · 2017-09-05T17:35:01Z

Had to do some significant rebasing; hopefully all sorted now

fungjj92 added the in review label Aug 31, 2017

fungjj92 force-pushed the feature/jf/percentile-params branch 3 times, most recently from 463791f to 022d1c9 Compare August 31, 2017 17:32

fungjj92 requested a review from flibbertigibbet August 31, 2017 17:33

fungjj92 assigned flibbertigibbet Aug 31, 2017

flibbertigibbet suggested changes Aug 31, 2017

View reviewed changes

fungjj92 force-pushed the feature/jf/percentile-params branch 2 times, most recently from 0e8a1b2 to 1815c8a Compare August 31, 2017 19:57

fungjj92 force-pushed the feature/jf/percentile-params branch from 1815c8a to c8bd5b6 Compare September 1, 2017 16:27

flibbertigibbet reviewed Sep 1, 2017

View reviewed changes

CloudNiner reviewed Sep 1, 2017

View reviewed changes

fungjj92 force-pushed the feature/jf/percentile-params branch from c8bd5b6 to 5906d6d Compare September 1, 2017 18:38

flibbertigibbet reviewed Sep 1, 2017

View reviewed changes

fungjj92 force-pushed the feature/jf/percentile-params branch from 1ef7c11 to 0b9192f Compare September 1, 2017 20:55

flibbertigibbet approved these changes Sep 2, 2017

View reviewed changes

flibbertigibbet mentioned this pull request Sep 2, 2017

Add historic range control #240

Merged

CloudNiner approved these changes Sep 5, 2017

View reviewed changes

flibbertigibbet mentioned this pull request Sep 5, 2017

Add form validation feedback #243

Open

flibbertigibbet force-pushed the feature/jf/percentile-params branch from 0b9192f to 26c02f3 Compare September 5, 2017 15:39

fungjj92 added 2 commits September 5, 2017 19:34

Add percentile extra params

2793fb2

Enforce whole number percentiles

1f682f9

Rebase ontop basetemp params and factor out constants

f35712f

flibbertigibbet force-pushed the feature/jf/percentile-params branch from 26c02f3 to f35712f Compare September 5, 2017 17:34

Linting

ed9faac

flibbertigibbet merged commit 0927c2c into develop Sep 5, 2017

hectcastro removed the in review label Sep 5, 2017

flibbertigibbet deleted the feature/jf/percentile-params branch September 5, 2017 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add percentile extra params #230

Add percentile extra params #230

fungjj92 commented Aug 31, 2017 •

edited

Loading

flibbertigibbet left a comment

flibbertigibbet Aug 31, 2017

flibbertigibbet Aug 31, 2017

flibbertigibbet Aug 31, 2017

flibbertigibbet Aug 31, 2017

flibbertigibbet Aug 31, 2017

flibbertigibbet commented Aug 31, 2017

fungjj92 commented Sep 1, 2017

flibbertigibbet commented Sep 1, 2017

fungjj92 commented Sep 1, 2017

flibbertigibbet left a comment

CloudNiner left a comment

CloudNiner Sep 1, 2017

flibbertigibbet Sep 1, 2017

fungjj92 commented Sep 1, 2017

flibbertigibbet left a comment

CloudNiner left a comment

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

Add percentile extra params #230

Add percentile extra params #230

Conversation

fungjj92 commented Aug 31, 2017 • edited Loading

Overview

Demo

Notes

Testing Instructions

flibbertigibbet left a comment

Choose a reason for hiding this comment

flibbertigibbet Aug 31, 2017

Choose a reason for hiding this comment

flibbertigibbet Aug 31, 2017

Choose a reason for hiding this comment

flibbertigibbet Aug 31, 2017

Choose a reason for hiding this comment

flibbertigibbet Aug 31, 2017

Choose a reason for hiding this comment

flibbertigibbet Aug 31, 2017

Choose a reason for hiding this comment

flibbertigibbet commented Aug 31, 2017

fungjj92 commented Sep 1, 2017

flibbertigibbet commented Sep 1, 2017

fungjj92 commented Sep 1, 2017

flibbertigibbet left a comment

Choose a reason for hiding this comment

CloudNiner left a comment

Choose a reason for hiding this comment

CloudNiner Sep 1, 2017

Choose a reason for hiding this comment

flibbertigibbet Sep 1, 2017

Choose a reason for hiding this comment

fungjj92 commented Sep 1, 2017

flibbertigibbet left a comment

Choose a reason for hiding this comment

CloudNiner left a comment

Choose a reason for hiding this comment

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

flibbertigibbet commented Sep 5, 2017

fungjj92 commented Aug 31, 2017 •

edited

Loading