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

Update MMM-ArduPort.js #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

LilCritical
Copy link

Fix for not showing data on mirror

@Dentrax
Copy link
Owner

Dentrax commented Feb 1, 2022

Thanks for PR @LilCritical, would you mind adding a working screenshot? 🙏

Copy link
Owner

@Dentrax Dentrax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added some reviews.

MMM-ArduPort.js Outdated
Comment on lines 253 to 255
if(sensor.name == "LM35") {val.innerHTML = sensor.value + " °C";}
if(sensor.name == "MQ2") {val.innerHTML = sensor.value + " %";}
if(sensor.name == "HCSR04") {val.innerHTML = sensor.value + " cm";}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to hard-code the formats here. We have format options, see here: https://github.com/Dentrax/MMM-ArduPort#configuration-options

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardcoding this is what fixed this issue since
formatValue: function(value, sensor) {
is not working properly ( value remains undefined )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, got it. Instead of applying a workaround, have you enough free time to find the root cause and apply the fix in the correct place? I would be very grateful if you could give a hand for this. 🙏

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, got it. Instead of applying a workaround, have you enough free time to find the root cause and apply the fix in the correct place? I would be very grateful if you could give a hand for this. 🙏

i will try to fix , but i need todo more research , fairly new to this type of coding.
but i am familiar with python and js , but more in bot developing

MMM-ArduPort.js Outdated

description.innerHTML = sensor.description;
row.appendChild(description);
var description = document.createElement("div");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider add new tab \t here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this i forgot to add

@LilCritical
Copy link
Author

Thanks for PR @LilCritical, would you mind adding a working screenshot? 🙏

Hello ,
As requested.
646342C3-4C9B-4927-B8E8-2367AE74C1D0
47EF85D1-93ED-470D-A7CA-D555B4B30B48

Thanks for your review i will look into it after work ✌️

fix without hardcoding
fix without hardcoding sensors
@Dentrax Dentrax mentioned this pull request Feb 21, 2022
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.

2 participants