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

Dynamic Properties - Ensure consistency on how dynamic classes (and other props) are set #497

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

joepavitt
Copy link
Collaborator

@joepavitt joepavitt commented Jan 18, 2024

Description

Lots more work in here than originally planned:

  • Update how we manage dynamic properties, moving away from the topic/payload structure we'd previously defined whereby topic had to be ui-property:<property> - this was limiting to a single property each message, and prevented the use of topic for other functionality.
  • Updated all documentation to reflect the widgets that support dynamic classes, for both online docs and in-editor docs
  • Fix dynamic classes on all layouts for all widgets
  • Ensure the statestore is properly updated when dynamic properties are set, and that the returned .state object in a Widget is accurate, and not a hardcoded set of defaults
  • Update in-Editor help tooltip for dynamic class
  • Add in tests (and ensure support) for dynamic disabled/enabled widget states

Related Issue(s)

Closes #259
Closes #352

Checklist

@joepavitt joepavitt marked this pull request as ready for review January 19, 2024 10:34
@joepavitt joepavitt merged commit 81a7677 into main Jan 19, 2024
2 checks passed
@joepavitt joepavitt deleted the 259-button-class branch January 19, 2024 11:08
@colin-grierson
Copy link

This update seems to have broken things and I can't make the dynamic properties work either :-(

Broken:
I was using a number of Vuetify widgets, now almost all do not display

  • This is how it looks now
    image
  • Unfortunately I do not have a before image so I have to describe -
    At the bottom of each group should be a vuetify text box for entering a date and time with a 'Clear' icon to the left and a 'Search' icon to the right. Now all we see is the 'Clear' icon. The rest appears to be awol, not just hiding, as I cannot find the associated text in the html while I can find the text for the 'Clear icon'
    The ASP usage group should display a vuetify circular progress bar. Now all we see is the numeric component of this - '72'
    The html for this group is below, in the image for the text field properties

Trying to make the new control work
A text field, with dynamic properties, that was previously displaying correctly.

  • I changed the code to set msg.class instead of msg.payload and msg.topic
  • The code appears to be working because I see my class 'nr-metric-RED' in the HTML after other class names
  • But the text is not red
    Here is what I see in the inspect window
    image

Thanks for your help
Colin

@joepavitt
Copy link
Collaborator Author

joepavitt commented Jan 22, 2024

Hi @colin-grierson - so sorry for the inconvenience. I'm not working today, but at least wanted to do some information gathering before I work on this properly tomorrow.

Were the ui-template elements ever driven by dynamic visibility from your code at all?

For the dynamic class, am I right in saying that the CSS class is being added, but your custom CSS definitions are not being loaded at all?

These do have a common feature... the ui-template node. I have dozens of test templates that I run locally before pushing, and they're all working, so will need some more details from you if you're able to share back and forth as we investigate.

@joepavitt
Copy link
Collaborator Author

DO you see anything in the "Console" Tab of the developer tools, or in the Node-RED logs when you first start Node-RED?

@joepavitt
Copy link
Collaborator Author

joepavitt commented Jan 22, 2024

A quick test flow for the dynamic classes that's working for me in a new NR instance and with 0.11.6:

[
    {
        "id": "217f5470279801a8",
        "type": "ui-template",
        "z": "9392247661c4fd86",
        "group": "",
        "page": "",
        "ui": "ac76fc604d0aa66e",
        "name": "",
        "order": 0,
        "width": 0,
        "height": 0,
        "head": "",
        "format": ".test-class .nrdb-ui-text {\n    color: red;\n}",
        "storeOutMessages": true,
        "passthru": true,
        "resendOnRefresh": true,
        "templateScope": "site:style",
        "className": "",
        "x": 140,
        "y": 60,
        "wires": [
            []
        ]
    },
    {
        "id": "81e6cfc8ef2f900e",
        "type": "ui-text",
        "z": "9392247661c4fd86",
        "group": "913bf6dec8b2a204",
        "order": 0,
        "width": 0,
        "height": 0,
        "name": "",
        "label": "text",
        "format": "{{msg.payload}}",
        "layout": "row-spread",
        "style": false,
        "font": "",
        "fontSize": 16,
        "color": "#717171",
        "className": "",
        "x": 310,
        "y": 140,
        "wires": []
    },
    {
        "id": "593c64c94aeae2f2",
        "type": "inject",
        "z": "9392247661c4fd86",
        "name": "",
        "props": [
            {
                "p": "class",
                "v": "test-class",
                "vt": "str"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": false,
        "onceDelay": 0.1,
        "topic": "",
        "x": 130,
        "y": 140,
        "wires": [
            [
                "81e6cfc8ef2f900e"
            ]
        ]
    },
    {
        "id": "913bf6dec8b2a204",
        "type": "ui-group",
        "name": "Dynamic Class on UI Text",
        "page": "b06261546cb829cb",
        "width": "6",
        "height": "1",
        "order": -1,
        "showTitle": true,
        "className": "",
        "visible": "true",
        "disabled": "false"
    }
]

can you import that and try it out please? You will need to assign the imported group to a page within your Dashboard.

Something to note is that for the custom CSS class, I'm needing to over-define the scope with:

.test-class .nrdb-ui-text { ... }

so that it overrides the underlying Vuetify class since our dynamic classes actually get injected one layer higher, on a container, rather than on the widget itself. This hasn't changed in any recent update though, so shouldn't have been different.

@colin-grierson
Copy link

colin-grierson commented Jan 22, 2024 via email

@colin-grierson
Copy link

colin-grierson commented Jan 22, 2024 via email

@colin-grierson
Copy link

colin-grierson commented Jan 22, 2024 via email

@joepavitt
Copy link
Collaborator Author

Which version of Node-RED are you using @colin-grierson?

Can you try a method I've proposed here please: https://discourse.nodered.org/t/dashboard-2-ui-template-config-error/84785/4?u=joepavitt

Appreciate it'll be a pain with lots of ui-template nodes, but want to check if that is the same issue.

@colin-grierson
Copy link

colin-grierson commented Jan 23, 2024 via email

@colin-grierson
Copy link

colin-grierson commented Jan 23, 2024 via email

@joepavitt
Copy link
Collaborator Author

Dashboard V2 version? I loaded the latest version on Monday, 0.11.6

Hoping to find out which version of Node-RED you're running actually, was chatting with Nick yesterday about these ui-template problems, and there seems to be a verification bug in 3.1.3 that is causing us some grief.

@joepavitt
Copy link
Collaborator Author

Here is the definition of the circular progress bar

Very odd indeed, I copied that example exactly, and tried it out:

Screenshot 2024-01-24 at 07 26 50

@joepavitt
Copy link
Collaborator Author

joepavitt commented Jan 24, 2024

Wait, no I didn't copy it exactly, I surrounded it in <template><!-- your content here --></template> which is prescribed in the docs

If I remove the <template/> then I see the exact same behaviour as you do.

Not sure which version you were updating from? I think we used to support it without <template> back in 0.9.x, but when I introduced the new ui-template structure in 0.10.0, and the ability to define a full Vue component, it required the <template>, although I have to confess I thought it should have still worked without.

@joepavitt
Copy link
Collaborator Author

To address this too:

Why does the class definition in the template have two names – ‘.test-class’ and ‘.nrdb-ui-text’? My class definitions have just one name, for example ‘.nr-metric-RED’. I changed my style template to have two class names as in your example, deployed and now see colour :-)

In CSS you can define different layers of style, for example:

.my-class p {
   color: blue;
}```

would apply to all `<p>` elements inside your `.my-class`. Because it has two layers of depth to it's definition, if any styles clash, for example there was also a:

```css
p {
   color: red;
}

Then the "most defined" class wins - in this case, the paragraphs would render blue.

Vuetify, the library we are using to render a lot of our components provides a base set of styles and definitions, when we inject our own classes, we are actually injecting them into the parents of the widgets, not the widgets themselves, hence to need to "over define".

The more I think about it though, even though this will be more painful from a development effort, having to over-define is not intuitive at all, and will undoubtedly cause more headaches in the future for others. I'll see if I can fix that today.

@colin-grierson
Copy link

colin-grierson commented Jan 25, 2024 via email

@joepavitt
Copy link
Collaborator Author

Could I be a pain and ask you to open some posts onto https://discourse.nodered.org/tag/dashboard-2 please for each of the topics? That way, each topic can be focussed into a thread, and it puts it into a public forums where others can see the problems if they're facing similar issues.

The only thing I will quickly points out, but this may just be a copy+paste error, is that you're missing a closing } after your .nr-metric-RED .nrdb-ui-text class declaration, but before the nr-metric-RED .nrdb-ui-button, which may be why the latter is not loaded into the declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants