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.

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

Make SMBs and predictions fit better #2926

Merged
merged 38 commits into from Nov 1, 2017
Merged

Make SMBs and predictions fit better #2926

merged 38 commits into from Nov 1, 2017

Conversation

scottleibrand
Copy link
Member

This removes the leading 0 from boluses < 1U to make the labels fit better on closely spaced SMBs. It also adds a 2HR view, and limits the focus area to only display as many hours of purple prediction lines as the selected focusHours. This makes it possible to "zoom in" more on a period of interest.

@philipgo
Copy link
Contributor

I think this is a great enhancement for OpenAPS users. Limiting the forecast range is a very good idea.

The font size is perfect on desktop computers, but a bit too small on my iPhone using the Nightscout app.

For me the tooltips around the SMB entries could be even smaller. With the current size in the rendering branch it is still impossible to hover over some BG entries. Alternatively, this might be solved by positioning the tooltip target area for Bolus/Carb entries under the symbol (where the caption is) and for BG entries over the dot.

@scottleibrand
Copy link
Member Author

Is the font still too small after I bumped it up from 25 (in ZT) to 35 (in rendering)? I could try going back up to 40 (what's in dev) but that might start SMBs overlapping again, at least on anything longer than the 2h graph.

In the ZT branch I already narrowed the tooltip targets down to basically be the size of the dot plus the label (vs. having it be like 30m wide in dev). If you have a BG that's underneath a treatment dot, it won't be hoverable, but I don't think we want to try to "fix" that. We're already using the space above carbs for their labels and tooltip targets, and the space below insulin treatments for theirs. Not sure if there's any further tweaks that would be worthwhile, but if so you can play around with them at https://github.com/nightscout/cgm-remote-monitor/pull/2919/files#diff-de7f2411d8429f79b8d3436ca320c9dfR417 and PR something in if you find a better solution.

@scottleibrand
Copy link
Member Author

sorry, I forgot to actually push fb4e74e until just now. @drnoname82 can you try that?

@philipgo
Copy link
Contributor

I think the larger font size is not an improvement because it leads to overlapping and makes things harder to read (see screenshots). I would rather accept the smaller font size. Maybe a different font could lead to an even better result, but either way legibility is much better than in current dev.

I will play around with the tooltip targets. Probably this is not too important, but I would find it interesting to be able to delete single BG measurements much like other entries can be deleted by clicking and pulling them. This can only work if BG dots are not covered by tooltip targets.
iphone font size 25
iphone font size 35

@scottleibrand
Copy link
Member Author

In playing around with label placement (which led nowhere useful) I discovered that we weren't properly scaling the size of treatment bubbles. This PR now allows larger insulin-only and carb-only bubbles for boluses >1U and carb entries >20g (using the default CR). It also switches to using the profile CR if there's no treatment-specific CR (before defaulting to 20, as before if there's no profile). And since that made boluses half the size they were before (now using a CR of 10 instead of the default 20), I doubled the size (area) of the bubbles so that 0.1U microboluses are the same size as BG dots, which makes large meals and boluses quite a bit more visible (at the expense of covering up more, unless you use the 2H view). Thoughts?

@diabeticdude2
Copy link

diabeticdude2 commented Oct 18, 2017

looks great to me. I am able to see the info I need. I have tested larger fonts, (40) but when I use that the numbers start to overlap. 30 is a bit too small, 35 seems like the happy middle place and is readable on my phone.

@philipgo
Copy link
Contributor

This is very nice. I would still vote for the 25 font (or a better solution in case anyone has an idea) because at 30 SMBs given in two consecutive loops cannot be deciphered.

@scottleibrand
Copy link
Member Author

There will always be overlap at some zoom levels unless we figure out a new way to do labels. The question is how far you have to zoom in to see them all clearly. It should now be possible to read all the labels without overlap at least at the 2H zoom level. Is that sufficient?

I hope 35 is sufficiently large that non-OpenAPS users won't see a problem with it. If it is, we might have to figure out how to dynamically scale the font based on the number of treatments displayed.

@garykidd
Copy link

I like 25...but to each their own I suppose. Might also consider 1 hour interval....or 90 mins even if you are having issues with overlap.

@scottleibrand
Copy link
Member Author

Don't try 1.5h, or you'll have to find the variable in the Chrome debugger to change it back to a whole number to unbreak your NS. :-)

@cameronrenwick
Copy link

yup.. love it

This reverts commit ec25130.
We now use  npm-shrinkwrap.json
@PieterGit
Copy link
Contributor

PieterGit commented Oct 27, 2017

Made a start in updating this branch to Node 8.8.1 and fixing the tests.
PR #2991 has now been merged to this PR.

@scottleibrand
Copy link
Member Author

Now that the tests are fixed, what else needs to be done on this branch to get it into a suitable state for merge to dev?

@sulkaharo
Copy link
Member

Looks good to go but this now targets ZT, not dev? :)

@scottleibrand scottleibrand changed the base branch from ZT to dev November 1, 2017 19:54
@scottleibrand
Copy link
Member Author

Fixed.

@sulkaharo sulkaharo merged commit 9e0cc8b into dev Nov 1, 2017
@sulkaharo sulkaharo deleted the rendering branch November 1, 2017 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants