Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

Conversation

@wdmtech
Copy link
Contributor

@wdmtech wdmtech commented Oct 14, 2020

  • Add color type to config manager
  • Add color picker to UI

😄

6771009005bcddb644a33acc22cd0223

Just a couple of things I'm not sure about:

  1. The optimal way to store a hex code in config
  2. Probably a better way of changing the to type="color" (also aware that I'm overwriting a const)

...For your deliberation!

@wdmtech wdmtech changed the title Feat add color type and picker Add color type and UI (native) color picker Oct 14, 2020
@wdmtech
Copy link
Contributor Author

wdmtech commented Oct 14, 2020

Looks like builds broke in #34

...Phew, it's not my fault!

@aaronse
Copy link

aaronse commented Oct 14, 2020

Nice to see color being added. I also added color, select and slider support in edits not yet pushed to my private branch.

Took slightly different approach and added optional "inputControl" attribute enabling decoupling of UI control definition from
config data type. Thoughts? Decoupling meant my web UI can optionally have slider ui control. Also, separation of UI control and data type allows for various numeric data types sizes. Pasted example snippet from recent project showing how <select> UI control is applied to specify fixed list of valid char array values.

Will try to contribute to this group effort and create pull request soon, fortunately/unfortunately my request will also include edits related to supporting ESP32 compilation (for subset of IOT framework's functionality, note SSL is missing).

Example config...

    {
        "name": "color",
        "label": "Custom Color",
        "type": "char",
        "length": 11,
        "inputControl": "color",
        "value": "#ff00ff"
    },
    {
        "name": "mode",
        "label": "Mode",
        "type": "char",
        "length": 20,
        "inputControl": "select",
        "value": "Aurora",
        "options": [
            "Aurora",
            "Blue Wave",
            "Custom Wave",
            "Sunset",
            "Ocean",
            "Rainbow",
            "White",
        ]
    },
    {
        "name": "brightness",
        "label": "Brightness",
        "type": "uint8_t",
        "inputControl": "slider",
        "value": 50,
        "min": 1,
        "max": 100
    },

@maakbaas
Copy link
Owner

maakbaas commented Oct 18, 2020

You guys are awesome, thanks for the input.

Looks like builds broke in #34

...Phew, it's not my fault!

Not sure what you mean, but the build was ok in that PR, and in the current master as well? The reason the build fails in this PR is because configuration.json is adjusted, but the example code is not updated to reflect this new structure.

Took slightly different approach and added optional "inputControl" attribute enabling decoupling of UI control definition from
config data type. Thoughts? Decoupling meant my web UI can optionally have slider ui control. Also, separation of UI control and data type allows for various numeric data types sizes. Pasted example snippet from recent project showing how <select> UI control is applied to specify fixed list of valid char array values.

Personally I am slightly in favor of this decoupled approach. @wdmtech what is your view on this?

One question I wanted to ask to both of you: is storing the color as the hex code the most sensible thing to do? How are you using the color in the rest of your code? Initially I was thinking to store the color as a RGB uint8 array, because that is how it would be used in most applications? But maybe there are other reasons why the hex string actually makes more sense to you (such as that int arrays are not yet supported by the configuration manager 😏).

Will try to contribute to this group effort and create pull request soon, fortunately/unfortunately my request will also include edits related to supporting ESP32 compilation (for subset of IOT framework's functionality, note SSL is missing).

I did start at ESP32 support myself, but never got to the end. I think it would be a great addition if you can create a PR for your implementation.

@wdmtech
Copy link
Contributor Author

wdmtech commented Oct 18, 2020

Personally I am slightly in favor of this decoupled approach. @wdmtech what is your view on this?

I am also in favor of this method! I can look at adapting my approach

One question I wanted to ask to both of you: is storing the color as the hex code the most sensible thing to do?

The truth is I'm a web app developer, so storing Strings is something I do at the drop of a hat 😆 But it's easy enough to convert between RGB and Hex so I'll look at storing as a RGB uint8 array if it would be more efficient.

@aaronse I take it your code is in your own branch? Perhaps I could take a peek and see if I can adapt it into a new PR - or is that something you'd like to do?

I'm only in my 3rd week programming with Ardino so consider me a novice, I don't know all the datatypes yet nor, how to use them effectively/efficiently.

my web UI can optionally have slider ui control

This is something I'd love to have in my projects too! So it seems as this would be the favorable approach, I'll have a play around and see what I can come up with.

Thanks guys!

@wdmtech
Copy link
Contributor Author

wdmtech commented Oct 18, 2020

Not sure what you mean, but the build was ok in that PR, and in the current master as well? The reason the build fails in this PR is because configuration.json is adjusted, but the example code is not updated to reflect this new structure.

OK, I misunderstood the build checks failing, I thought it was because of a recent breaking change unrelated to my PR, thanks for explaining

@aaronse
Copy link

aaronse commented Oct 26, 2020

Recently created #44 which adds ESP32 support (subset of functionality). The edits add slider, color, dropdown, toggle UI controls and some other bonuses.

@aaronse
Copy link

aaronse commented Oct 26, 2020

Hello @wdmtech ! Thanks for starting this pull request, you helped nudge me into getting around to submitting edits from my dev box and create #44

Am relatively new to React, so will appreciate yours (and others) help and contributions.

@maakbaas
Copy link
Owner

maakbaas commented Nov 6, 2020

@wdmtech I hope it is ok for you if I close this pull request in favor of the solution proposed by @aaronse. Really appreciate your contribution since it helped us discuss the best implementation. Thanks a lot.

@maakbaas maakbaas closed this Nov 6, 2020
@wdmtech
Copy link
Contributor Author

wdmtech commented Nov 6, 2020

Absolutely no problem!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants