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

When updating devices with readings seen since last update, convert F to C #4

Merged
merged 1 commit into from Apr 29, 2015

Conversation

phad
Copy link
Contributor

@phad phad commented Apr 27, 2015

Code in app/models/device_session.rb to handle client requests for new readings since last update didn't convert from F to C for users whose preference is C.

@hebnern
Copy link
Member

hebnern commented Apr 29, 2015

Sweet, thanks! I'll get this deployed soon.

hebnern added a commit that referenced this pull request Apr 29, 2015
When updating devices with readings seen since last update, convert F to C
@hebnern hebnern merged commit 2fea6e1 into brewbit:master Apr 29, 2015
@phad
Copy link
Contributor Author

phad commented May 2, 2015

I'm afraid this has introduced a new bug: when the new readings are added to the graph they are assumed to be fahrenheit, and go through a second conversion to celcius.
So the for celcius users the graph then dips down with the ajaxed-in readings.
A page refresh fixes it as then the whole graph is re-seeded with values from the readings file.
I didn't realise that temp conversion is also done in the inline JS in the _devices_session.html.erb file.
Looking at that now i think the correct fix would be to revert the change I made in device_session.rb and to add an <% if @device.user.temperature_scale == 'C' -%> section in the html.erb file to convert the most recent new reading to C just before updating the last-reading-n and last-setpoint-n DOM elements.

wdyt?

If you've not already deployed the previous fix can you hold off and I'll send a new PR with this different approach?

@phad
Copy link
Contributor Author

phad commented May 2, 2015

New PR with the alternative proposed fix: #5

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

2 participants