Multiple Changes #3
Conversation
v1.1 prep
Tvde1
left a comment
Tvde1
left a comment
There was a problem hiding this comment.
Many thanks for the effort you've put in! I'm sorry it took a while before looking at these changes. I don't visit github often and it just slipped my mind.
| version=1.0 | ||
| author=Tvde1 | ||
| maintainer=Tvde1 | ||
| version=1.1 |
There was a problem hiding this comment.
| version=1.1 | |
| version=2.0 |
It's quite a big change. Maybe use 2.0
There was a problem hiding this comment.
I also noticed the version is 1.1 already. If 2.0 doesn't sound good then go with 1.2.
|
|
||
| case VT_TEXT: | ||
| result += createInput(item.second, "text"); | ||
| //<input name=\"" + item.first + "\" type=\"text\" value=\""; |
There was a problem hiding this comment.
There is some commented out code here and at some other places. If the code won't be needed, remove it. If it could be needed in the future maybe add a comment explaining what it does and why it could be changed.
| Serial.println(name + "=" + val); | ||
|
|
||
| // skip empty input for values that are not displayed | ||
| if (item->second->hideValueInWeb && val == "") { |
There was a problem hiding this comment.
What happens when item->second->hideValueInWeb is true but the value is not ""? Can that happen?
| virtual void serialize(JsonObject*) = 0; | ||
| virtual void deserialize(JsonObject*) = 0; | ||
| bool hideInWeb = false; | ||
| bool hideValueInWeb = false; |
There was a problem hiding this comment.
What's the use cases of hideInWeb and hideValueInWeb? Why add it to the config tool if it won't be changed?
I could see a use case for showing the value but making it read-only (e.g. textbox is greyed out). That way you can show like the hostname/MAC of the ESP which could help in the process of changing the configuration.
| @@ -0,0 +1,113 @@ | |||
| /** | |||
| * Test functionality of ConfigTool. | |||
There was a problem hiding this comment.
Nice, a unit test! I wonder why I hadn't created one :)
| *.pyc | ||
| *.pyc | ||
|
|
||
|
|
There was a problem hiding this comment.
Maybe add a comment for why this is added
| @@ -0,0 +1,13 @@ | |||
| # main class | |||
| maintainer=Tvde1 | ||
| version=1.1 | ||
| author=Tvde1,gerdlanger | ||
| maintainer=gerdlanger |
There was a problem hiding this comment.
| maintainer=gerdlanger | |
| maintainer=Tvde1 |
I very much appreciate the effort you have put in so it's very fair you are listed as author but I think that I should still be listed as the maintainer. I promise I'm not gonna abandon this library anytime soon :)
| @@ -1,24 +1,29 @@ | |||
| /* | |||
| Name: ConfigTool.cpp | |||
| Author: Tvde1 | |||
2 participants
Uh oh!
There was an error while loading. Please reload this page.