weather test failed

weather test failed

this one weather test seems to fail very often, don't know why ... (restarted it)

will update doc, to add 2 parms, hoursColor, minutesColor (like secondsColor)
only used if changed,
added css example, (commented out)

Just a thought: Instead of adding new options to the config for the hour and minute color. What do you think about marking secondsColor as deprecated and just moving the color (for hours, minutes and seconds) into the css file? If someone wants a different color, they can change the custom.css. Or am I missing some advantage of defining this in the config?

I hope I'm not annoying you as I often ask such questions 😅

not annoying at all. more eyes and questions improve the results

deprecated as in not documented, replaced by css
(need still defined for mmm-config type config options, else lost on save) i am ok with

i had css only before this push, but asked for guidance on consistency.

weather failed again

weather failed again

you have the rights to restart (failed) tests

yes i know. waiting til we come to consensus on which way to go

not annoying at all. more eyes and questions improve the results

Nice 😃

deprecated as in not documented, replaced by css

Yes. And maybe if this option is set, a deprecation warning so we could after some releases may remove this option at all.

i had css only before this push, but asked for guidance on consistency.

Yes, it should be consistent, either via config options or via css.

Yes, it should be consistent, either via config options or via css.

its both before my changes now

we could only do the deprecation message with lint as the module is in browser only. so
messages go to browser log which is hardly ever reviewed

we could only do the deprecation message with lint as the module is in browser only.

We could also do it in the check_config.js 🙂

I just had a closer look. It turned out that in check_config.js it wouldn't be so easy, but I noticed that we already have a checkDeprecatedOptions function in the app.js (to check deprecated options in the core) 💡. So I just extended it to check for deprecated options in the modules.

Checkout this commit in my testing branch:
KristjanESPERANTO@f34f2b8

What do you think?

@rejas @khassel what do you think,
i like the idea of moving to css

Both solutions have its dis/advanteges.
A config option doesnt require much knowledge of the user in css
A css styling option is much cleaner in the code

So I dont mind going the css route, depreacting the config options and adding the css styling as a new option into the docs of the module

@KristjanESPERANTO I want to add an entry to the clock_styles.css for the three classes..but empty

eslint rejects that..

what is a good approach to bypass this error?

the reason for adding is that there are already classes for the analog clock arms,
and these are different names..
analog
clock-minute,hour, second

there is already a set of classes set on the div around the time
timeWrapper.className = "time bright large light";

digital

.module.clock .clock-hour-digital {
  
}
.module.clock .clock-minute-digital {

} 
.module.clock .clock-second-digital {

}

@sdetweil I took a look at it and made a few changes. I've set a PR on your branch so that we don't have to talk in theory for too long: https://github.com/sdetweil/MagicMirror/pull/57.

what is a good approach to bypass this error?

Instead of bypass it and using an empty class, I continued the approach of doing CSS stuff in CSS instead of JavaScript.

What do you think of it?

now, can we write a testcase to prove the two spans instead of the time string. i hadnt made it there yet

i submitted a pr to documentation for this deprecated property and added info on css