CGM Trends Range Selections #36
Conversation
krystophv
changed the title
|
Is dependency of: tidepool-org/blip#363 |
|
ping @jebeck for review (or feel free to pass to next available person) |
There was a problem hiding this comment.
Two overall requests: (1) the small change to the arguments to connect in RangeSelect - the () => {} is actually worse than I thought (it throws up a bunch of errors in the console) and (2) a discussion/sync up about future approach in general (deeply nested connect()-ed components or not) and the path forward for controlling Trends options via checkboxes through redux state (b/c the RangeSelect component isn't quite the right interface for the BGM options)
|
|
||
| .label { | ||
| composes: mediumContrastText from '../../../styles/typography.css'; | ||
| font-size: 16px; |
There was a problem hiding this comment.
Can we try to keep font sizes out of the component-level CSS files, please? Create a new variable in the typography.css if needed?
| }, dispatch); | ||
| } | ||
|
|
||
| export default connect( |
There was a problem hiding this comment.
A couple of comments here: the first and third functions can just be absent (can pass null for the first and just leave the third out - if you were looking at the TrendsContainer example, it adds an additional property in the third function ({withRef: true}) for compatibility with blip's parent-child component communication through refs (an outdated strategy), but nothing like that is necessary here). Generally connect()-ed components are also considered containers, not components.
Back in the baby days of redux, it wasn't considered very good practice to nest a lot of connect()-ed components, but apparently the advice has changed a little (linking for Dan's comment, his link is now broken). However, it does sound like there are still some potential performance issues, for which there is a potential middleware solution.
Since we're starting to be concerned about performance here in viz, I think it'd be good to discuss pros & cons of the options here and sync up on future strategy (with @hntrdglss as well). Also more generally the approach you've taken here is not what I expected. Since we already had code in blip that renders checkboxes for the BGM options and since the checkboxes have to be rendered in blip code due to the current split between what blip controls of the dataviz options, I expected that code to be reused (and there was a comment about this on the card ("...the checkboxes should use the same code as the checkboxes for the options available on BGM trends (Lines on/off, Range & Avg on/off, etc.)" from Nov. 8 when @darinkrauss was helping to get things more precise on the card). I get that the Trello cards are full of lots of info in a not easily digestible format, so I'm not surprised you missed that, but it might have been nice to talk through the approach before the PR was up to make sure we're all on the same page about approach and have any discussions about pros & cons of options with respect to future code.
|
@jebeck - addressed the CSS issue and leaving the |
Conflicts: package.json src/containers/trends/CBGTrendsContainer.js src/containers/trends/TrendsContainer.js src/index.js src/redux/actions/trends.js src/redux/constants/actionTypes.js
Allows the user to toggle the display of individual ranges in the CGM Trends view.