Web UI improvements

[factormystic]

todo

• disable WiFi input auto capitalization
• trim WiFi form inputs
• show project name in page header (instead of ESP8266)
  • moved to Show projectName config as header title factormystic/esp8266-iot-framework#1 (which is based on this branch for now, but can be rebased after this is merged)
• add an optional "display name" configuration property
• separate <input> types for different configuration data types (eg, char -> text, bool -> checkbox, int/float -> numeric)
• add maxlength attribute for char types, set to configured length
• figure out how to get a disabled attribute into these config inputs (and ideally, don't persist them if sent)
• consider if number types should have a min/max/step, and if this can be enforced
• show min/max next to config labels
• infer min/max based on data type, unless provided
• explicit step value of 1, unless provided
• add a "hidden" option to avoid displaying the input control

discuss

• ✔ linting configuration (eg, eslint)
  • will be future PR
• ✔ why use document.querySelector(...).value for form inputs rather than data binding?
  • okay to refactor, but separate PR
• ✔ thoughts on using actual true/false for boolean config rather than strings
  • agreed it was a good change

[maakbaas]

Great changes. Some of them were already on my list, but I did not get to them yet, but many of them I had not even thought of. In general I would merge this pull request, except for two points:

1. Regarding the project name in the header: I like this idea, but what I don't like is that it is read straight from the JSON. This means that if somebody changes the project name in the GUI, the header will not be updated. Furthermore right now it is a hard requirement to have this project name as a configuration item. My proposal would be to read the project name dynamically, and include a fall back to for instance ESP8266 in case of an error, or in case the project name is not defined. And then add an explanation about this in the documentation.

2. I like being able to give a configuration item a different name for the GUI, I also see the benefit in removing the data type for visual consistency, but personally I think it could be handy to see this data type in the interface, also to distinguish between different type of integers. Another way that would be acceptable for me could be to give a default min max for the different integer types, and a default step of 1.

I am curious what your thoughts are, and of course I can also work on this once we agree on an approach :).

[maakbaas]

Also, regarding your three discussion points:

• linting configuration (eg, eslint)

I am less familiar in this area, so if you have a proposal I am all ears.

• why use document.querySelector(...).value for form inputs rather than data binding?

Probably because this is the approach I was most familiar with (I am self-taught in JS). I am open for improvements, as long as it does not add a lot of code/dependencies, to keep the footprint as low as possible

• thoughts on using actual true/false for boolean config rather than strings

I see you already implemented this, and I agree this is much better than the strings approach!

[factormystic]

1. Regarding the project name in the header: I like this idea, but what I don't like is that it is read straight from the JSON. This means that if somebody changes the project name in the GUI, the header will not be updated. Furthermore right now it is a hard requirement to have this project name as a configuration item. My proposal would be to read the project name dynamically, and include a fall back to for instance ESP8266 in case of an error, or in case the project name is not defined. And then add an explanation about this in the documentation.

Good point. I'll move that commit to a separate PR draft for the time being and we can brainstorm about the right way to implement it. I did notice that projectName was mandatory config, but at least this specific code would work even if it was missing (it'd just be a blank header).

2. I like being able to give a configuration item a different name for the GUI, I also see the benefit in removing the data type for visual consistency, but personally I think it could be handy to see this data type in the interface, also to distinguish between different type of integers. Another way that would be acceptable for me could be to give a default min max for the different integer types, and a default step of 1.

I suppose it's a question of who this page is for in an ideal project. Implementers know the data types --- they're the ones that wrote the config file. So I view it as a place for device users instead (and for small projects, that's the same person). I agree that showing the min/max in the UI would be nice, so I'll bring that back. The default step value in browsers is 1, so I think that's covered although the component could also provide an explicit fallback when undefined. Using a default min/max based on data type is a great idea, I assume it's possible right now to enter negative numbers for uints which is unhelpful.

Also looking at MDN docs for number inputs I noticed that there's a special any value for step, which means floats probably can use number inputs and therefore min/max. I'll give this a try too.

I am curious what your thoughts are, and of course I can also work on this once we agree on an approach :).

Sounds good 👍

(linter) I am less familiar in this area, so if you have a proposal I am all ears.

Basically what I think we'd want is to add a dev dependency on eslint, and a reasonable initial config file (such using their recommended defaults, and probably also eslint-plugin-react). The benefit is primarily so that contributors know what style to write code, but also to help avoid common pitfalls and make code quality improvements (eg, recommending const instead of var for variables that can be constants). This is VERY nice when combined with editors such as VS Code, where it can integrate directly and show recommendations in real time. I'll open a separate PR that does this, and then run the linter on the javascript, and you can see how you feel about that change independent of improvements in this PR.

Ideally we could do this for the cpp code as well, I had been looking into clang-tidy but I'm much less familiar with that part of the stack and I haven't yet got it set up.

(binding) Probably because this is the approach I was most familiar with (I am self-taught in JS). I am open for improvements, as long as it does not add a lot of code/dependencies, to keep the footprint as low as possible

No sweat, I'm familiar with JS patterns but haven't written much React (besides tutorials), so this is pretty fun for me to figure out. I believe that this would be a minor refactor inside the React components themselves without any new dependencies. I also don't expect much/any code size change. I'll put that in a separate PR as well so it can be considered separately.

(bools) I see you already implemented this, and I agree this is much better than the strings approach!

👍

[maakbaas]

Good point. I'll move that commit to a separate PR draft for the time being and we can brainstorm about the right way to implement it. I did notice that projectName was mandatory config, but at least this specific code would work even if it was missing (it'd just be a blank header).

You are right that this parameter was already 'mandatory', because it is used in main.c, I forgot about that. Still I think of the main.c more as a template/getting started file, which would maybe be easier to change or modify by the user if they would want to remove projectName from the template configuration for some reason. I expect the typical user to be more skilled in the embedded side of things than the front-end side.

I suppose it's a question of who this page is for in an ideal project. Implementers know the data types --- they're the ones that wrote the config file. So I view it as a place for device users instead (and for small projects, that's the same person). I agree that showing the min/max in the UI would be nice, so I'll bring that back. The default step value in browsers is 1, so I think that's covered although the component could also provide an explicit fallback when undefined. Using a default min/max based on data type is a great idea, I assume it's possible right now to enter negative numbers for uints which is unhelpful.

Yes, very good points as well. For the first use case in which I want to use the framework myself, I had actually foreseen to maybe make a custom configuration page tailored more towards end-users. But your addition of the separate name field got me thinking that that would not be necessary. By adding for instance also a description field, or on top of the 'disabled' property a 'hidden' property, the page could be configured and tailored more towards the end-user application.

Haha, so many ideas right now. For my application I would need a color picker, but that can potentially also be added as a 'color' datatype that will add a color picker to the interface.

Basically what I think we'd want is to add a dev dependency on eslint, and a reasonable initial config file (such using their recommended defaults, and probably also eslint-plugin-react). The benefit is primarily so that contributors know what style to write code, but also to help avoid common pitfalls and make code quality improvements (eg, recommending const instead of var for variables that can be constants). This is VERY nice when combined with editors such as VS Code, where it can integrate directly and show recommendations in real time. I'll open a separate PR that does this, and then run the linter on the javascript, and you can see how you feel about that change independent of improvements in this PR.

Sounds awesome. I have some experience with linters in other languages, but did never set up eslint in my own toolchain yet.

Thanks a lot for your enthusiasm, and don't hesitate to let me know if and where I can assist (I am also learning a lot about Git, since I've mainly worked with SVN in the past and yours have been my first pull requests ;)).

[factormystic]

You are right that this parameter was already 'mandatory', because it is used in main.c, I forgot about that. Still I think of the main.c more as a template/getting started file, which would maybe be easier to change or modify by the user if they would want to remove projectName from the template configuration for some reason. I expect the typical user to be more skilled in the embedded side of things than the front-end side.

I think another part of it is, what is the expectation for how this project should be used? There's a few ways to think about it. For example, people who want to build apps on this framework might clone/fork and start editing/customizing whatever they want. Of course that's allowed, but it might not be helpful especially if people want to incorporate core changes into their projects later on. I'm still figuring that out myself.

One idea I had is to have people start a fresh repo for their own projects instead of fork, but then add this code as a git submodule. To reference an inspiration, this is how themes for the static blogging program Hugo typically work (explanation). The obvious problem is that's a pure web project whereas this is a compiled IoT project so I actually don't know if it can work, but conceptually I like the idea of pulling this project in as a dependency somehow, then in my own repo I have only my own configuration.json, main.cpp, etc, and can easily pull in updates.

I expect the typical user to be more skilled in the embedded side of things than the front-end side.

It's actually the opposite for me! Web stuff is my day job (although not React), and IoT is the hobby (keep that in mind when I send a future PR to make some cpp changes 😂...)

By adding for instance ... a 'hidden' property

Done

yours have been my first pull requests

Haha, congrats I guess. I had been thinking about a possible project for my ESP8266 and when I saw your project here show up on Hack-a-day recently I thought I would see if I could get it running. And that post mainly focused on making HTTPS calls w/o hardcoding cert thumbprints and didn't even mention the self-hosted config dashboard!

[factormystic]

Here's another question: Is the current handling of length from configuration.json correct? That defines the length of the char array in config.h, and that's supposed to be inclusive of a null byte, correct? Eg, if you define a string with "length": 32, should the actual max input value be 31? I'm actually not sure if that's true, just speculating?

[maakbaas]

I think another part of it is, what is the expectation for how this project should be used? There's a few ways to think about it. For example, people who want to build apps on this framework might clone/fork and start editing/customizing whatever they want. Of course that's allowed, but it might not be helpful especially if people want to incorporate core changes into their projects later on. I'm still figuring that out myself.

One idea I had is to have people start a fresh repo for their own projects instead of fork, but then add this code as a git submodule. To reference an inspiration, this is how themes for the static blogging program Hugo typically work (explanation). The obvious problem is that's a pure web project whereas this is a compiled IoT project so I actually don't know if it can work, but conceptually I like the idea of pulling this project in as a dependency somehow, then in my own repo I have only my own configuration.json, main.cpp, etc, and can easily pull in updates.

Yes, it's again a good point. I will give it some thought. Another option could potentially be to make it into a PlatformIO library so that you can include it into your project in that way. But I need to look into this if among other things, you can then still have the benefits of the build hooks straight from a library.

It's actually the opposite for me! Web stuff is my day job (although not React), and IoT is the hobby (keep that in mind when I send a future PR to make some cpp changes 😂...)

I figured as much :)

Thanks, I have merged the pull request.

[maakbaas]

Here's another question: Is the current handling of length from configuration.json correct? That defines the length of the char array in config.h, and that's supposed to be inclusive of a null byte, correct? Eg, if you define a string with "length": 32, should the actual max input value be 31? I'm actually not sure if that's true, just speculating?

You are right, fixed it in afd78e0

[maakbaas]

Finally got to implementing projectName as a title, as you did suggest here as well. Thanks :).