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

Fixed nested arrays settings #1504

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Fixed nested arrays settings #1504

merged 3 commits into from
Jul 19, 2023

Conversation

Slonick
Copy link

@Slonick Slonick commented Jun 3, 2023

♻️ Current situation

Nested arrays display only one element.

💡 Proposed solution

To solve the problem, the generated layout and data are matched.

⚙️ Release Notes

The fix is applied only if "fixArrays": true is added to config.schema.json.
For example:
{ "pluginAlias": "examplePluginAlias", "pluginType": "platform", "singular": true, "fixArrays": true, "schema": {...} }

@donavanbecker
Copy link
Contributor

Please re-branch to beta branch

@Slonick Slonick changed the base branch from master to beta-4.50.5 June 3, 2023 18:28
@bwp91
Copy link
Contributor

bwp91 commented Jul 3, 2023

Hi @Slonick
could you show a screenshot of how this would look in the ui?
My opinion personally (not necessarily homebridge-opinion!) is that it would be nice to avoid extra options like fixArrays in the config, I totally appreciate why you have added this, assuming for backward compatibility?

@grzegorz914
Copy link
Contributor

@donavanbecker , @bwp91

Why this can not be merged, if you use tab array in layout then only one element is displayed however many are configured, I think this resolve this:
grzegorz914/homebridge-denon-tv#92
and this:
grzegorz914/homebridge-lgwebos-tv#95

@donavanbecker
Copy link
Contributor

@bwp91 I would agree with @grzegorz914. I have experienced this issue as well.

@donavanbecker
Copy link
Contributor

@Slonick Once merged can you also add something to config wiki explaining this?

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

@Slonick Once merged can you also add something to config wiki explaining this?

This fix requires additional testing, I checked on my plugin and on 1-2 more from open issues, so I added a separate flag. Later it will not be needed, so I think it makes no sense to describe it in the wiki.

@donavanbecker
Copy link
Contributor

@Slonick Okay so this PR isn’t ready?

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

@Slonick Okay so this PR isn’t ready?

what do mean PR?

@donavanbecker
Copy link
Contributor

Pull Requests

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

Pull Requests

no, it's ready for pull request. It's not ready for release. I need feedback that it works. And it works not only for nested arrays, but also does not break anything on regular configs using the "fixArrays": true.

@grzegorz914
Copy link
Contributor

grzegorz914 commented Jul 19, 2023

It will be good merge it to beta branch and publish new beta to test.

@donavanbecker donavanbecker merged commit 7f2fa91 into homebridge:beta-4.50.5 Jul 19, 2023
@donavanbecker
Copy link
Contributor

@Slonick New beta should be published shortly

@donavanbecker
Copy link
Contributor

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

@Slonick Eslint failed. Can you submit another Pull Request?

https://github.com/homebridge/homebridge-config-ui-x/actions/runs/5601841759/jobs/10246388470#step:5:1

yes i will do it soon

@donavanbecker
Copy link
Contributor

test.5 has been released.

@donavanbecker
Copy link
Contributor

@Slonick, so "fixArrays": true just needs to be added to the base of config.schema.json? So right after this line: https://github.com/0x5e/homebridge-tuya-platform/blob/fd48de4007918b32be22045e5b6a20121b69e178/config.schema.json#L4

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

@Slonick, so "fixArrays": true just needs to be added to the base of config.schema.json? So right after this line: https://github.com/0x5e/homebridge-tuya-platform/blob/fd48de4007918b32be22045e5b6a20121b69e178/config.schema.json#L4

yes, that's right, and after that the arrays should be displayed correctly

@donavanbecker
Copy link
Contributor

Screenshot 2023-07-19 at 5 50 14 PM I just tested this out with out luck. I will try it under the Array.

@donavanbecker
Copy link
Contributor

no luck moving it to another section

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

@donavanbecker Are we talking about the same issue? If you look at my screenshot from the second version of pull request, then there is MacBook and PC, before the fix the tab from the PC was not displayed.

@Slonick
Copy link
Author

Slonick commented Jul 19, 2023

if you have duplicate data or data is not displayed, but it is in config, then I need name of plugin and your current config, and tomorrow I will check it.

@donavanbecker
Copy link
Contributor

Yes it does look the same.

Plugin: https://github.com/0x5e/homebridge-tuya-platform/blob/develop_1.7.0/config.schema.json
Config:

{
            "name": "Tuya",
            "options": {
                "projectType": "2",
                "accessId": "xxxxxxxxxxxx",
                "accessKey": "xxxxxxxxxxxxxxxxxxxxxxxx",
                "countryCode": 1,
                "username": "[email protected]",
                "password": "password",
                "appSchema": "tuyaSmart",
                "homeWhitelist": [
                    12345678
                ],
                "deviceOverrides": [
                    {
                        "id": "global",
                        "unbridged": true,
                        "schema": [
                            {
                                "code": "motion_switch",
                                "hidden": true
                            },
                            {
                                "code": "floodlight_switch",
                                "hidden": true
                            },
                            {
                                "code": "floodlight_lightness",
                                "hidden": true
                            }
                        ]
                    }
                ],
                "debug": true
            },
            "platform": "TuyaPlatform"
        },

@bwp91
Copy link
Contributor

bwp91 commented Jul 20, 2023

I haven’t tested this - but hypothetically (i know it’s already been merged) what would happen if this PR didn’t have the config option and it were to be merged without the config option?

@grzegorz914
Copy link
Contributor

grzegorz914 commented Jul 20, 2023

I have tested this on my plugins and looks like all is correct now, before was only 1 element visible.

IMG_0876

@donavanbecker
Copy link
Contributor

@grzegorz914, what change did you have to make?

@grzegorz914
Copy link
Contributor

@donavanbecker
I have added to my config.schema „fixArrays”: true, before apply the fix TabArray displayed only 1 element, now displays all as you can see on the screenshot. This problem was if you use config.schema with layout.

@Slonick
Copy link
Author

Slonick commented Jul 20, 2023

Screenshot 2023-07-19 at 5 50 14 PM I just tested this out with out luck. I will try it under the Array.

image

i fixed it

donavanbecker added a commit that referenced this pull request Aug 19, 2023
* Add singular plug-in label (#1458)

* Document the default port/address that you can access the UI in your web browser (#1252)

* Improve translations (#1459)

* Fix capitalization (#1496)

* Update zh-CN.json (#1503)

* Fixed nested arrays settings (#1504)

* Fixed nested arrays settings #2 (#1510)

* Fixed nested arrays;
Fixed lint;

* Removed console.log()

* Fixed nested arrays #3 (#1511)

* Wait for custom UI (#1522)

* Wait for custom UI (#1523)

---------

Co-authored-by: Dave Nicolson <[email protected]>
Co-authored-by: Adam Coulombe <[email protected]>
Co-authored-by: Aiden <[email protected]>
Co-authored-by: Artem Kononenko <[email protected]>
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.

5 participants