Feature/multi chart scrubber #104
Conversation
|
Won't have a chance to look at this before RRP, feel free to reassign as necessary. |
|
That's okay, no rush on it. |
| private multiChartScrubberInfo = new Subject(); | ||
| private multiChartScrubberHover = new Subject<Boolean>(); | ||
|
|
||
| multiChartScrubberInfo$ = this.multiChartScrubberInfo.asObservable(); |
There was a problem hiding this comment.
Is this $ postfix a common convention in the rxjs world? If it isn't, it may be better to just make the public property just be multiChartScrubberInfo or multiChartScrubberObservable and then give the private var the same name with an _ prefix in order to be consistent with existing JS naming conventions.
There was a problem hiding this comment.
I saw the $ in a lot of use cases and some Angular documentation, but no explicit mention of it being the convention. In fact, I can't find any convention documentation anywhere.
OTM and Civic Apps use "Stream" as the (bacon.js) observable postfix and find it helpful for readability. This is something they came up with on their own as opposed to universal standards.
"Observable" seems clear to me for now until/unless we decide otherwise
There was a problem hiding this comment.
Looks like it's a pattern from Cycle.js, I've seen it used in a couple places but it seems to stem from them.
There was a problem hiding this comment.
Nice find @rmartz. I personally quite like the $ and would adopt it happily if the team was into it. Otherwise, nameObservable or nameStream are clear to me.
| @@ -13,8 +13,23 @@ import * as _ from 'lodash'; | |||
| @Injectable() | |||
| export class ChartService { | |||
|
|
|||
| private multiChartScrubberInfo = new Subject(); | |||
There was a problem hiding this comment.
A subject seems pretty correct for this implementation. While I have limited experience as well, Subjects can be frowned upon as they have state, which is no good for the functional programming paradigm, and so see limited use.
I found this, appears to be a great deep dive into when to and when to not use Subjects:
http://davesexton.com/blog/post/To-Use-Subject-Or-Not-To-Use-Subject.aspx
Overview
Multi chart scrubber for yearly indicators!
Demo
### NotesMulti-scrubber depends upon 2 Observable streams. One listens for any chart #overlay being moused over and relays it to every line graph component. The other listens for mouse positioning and relays that to every line graph component to redraw the scrubber.
Testing Instructions
Toggle multi-chart scrubber in project settings and mouse over charts.
Closes #97