-
Notifications
You must be signed in to change notification settings - Fork 52
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
notification ui node enhancements #932
notification ui node enhancements #932
Conversation
I thought that the OK/Cancel feature was going to be handled by a new ui-dialog rather than being added to the notification node. |
OK, I have just seen the latest posts on #193 where it was decided to do it the way D1 did it. |
Co-authored-by: Joe Pavitt <[email protected]>
For the close via |
@joepavitt, |
Actually, let's go |
Should |
is it? |
Delay and Trigger nodes for example. |
Yes but inside the dashboard it is more common usage to clear stuff e.g. using an empty string in the payload (see dropdown, radiobuttons, ...) |
True. |
@joepavitt, However when I inject, Below the root cause is explained, but I don't know how to solve it. A bit too much Vue specific for me...
Perhaps I should only store the message in the else section, i.e. only store the message if it is going to be used to show a new dialog? |
I think there isn't much point saving the clear is there? If the page is refreshed while a notification is open it is not re-shown anyway. Plus if it had been cleared then it wouldn't be there anyway. |
@colinl, |
I'm happy to cover this @bartbutenaers Colin is right, we just want a conditional to check a |
Thanks @joepavitt !!! @colinl My issue from last night is another nice example of this discussion. I inject a message which does not contain any information about the notification title, and hence the title is gone. Because my message becomes the last stored message, and the widget expects to find ALL his information in that last message. But the last message only contains only a little part of all the required state information of course. And now you as a node developer need to start thing about which message should be stored and which one not. Only to make sure that the required information is available in the last stored message. Which imho is not a pretty solution anymore... I would expect that:
Sorry guys to repeat this theory over and over again. But I still wake up screaming in the middle of the night, being haunted by hundreds of replayed messages with faulty content ;-) |
As an example if you've defined You send a message which defined I'd say blue? In the pattern you've proposed though, I understand you'd ask for |
@joepavitt, |
should the command not be packed into Additionally, shouldnt we emulate the options for both close and confirm e.g. In fact, since it would be under |
@Steve-Mcl I had initially ruled that out because it wasn't persisting any changes, and note we changed it to
|
What are your thoughts @bartbutenaers @colinl ? |
@joepavitt
The new helper functions really simplify developing things like this!! Great job!! However the part that goes via the data store is still not clear to me. I currently only store the input message in the data store, if it contains a payload, because the payload contains the text that needs to be displayed. I have used the payload to send the text content, because I assume that that is the 'value' for this widget? Because that is the value that will be different between most notifications. Not sure if that is correct? Because I also need to show/hide the notification. Should that be a boolean dynamic property Hope that my question is a bit clear? I don't want the message to be displayed every time I inject a message to change some or another dynamic property... |
I think this is sensible given our current API
I'd be sending that via |
To explain my doubt a bit. I had understood the new way of working like this: all dynamic properties are stored in the state store. And the "value" of the widget is stored in the payload, so it will end up in the data store. But what is considerd as 'the value' of e.g. a notification widget? It could be the text that you display. But it could also be the fact whether you show or hide the notification. Would be nice to hear your thoughts about this... |
Hi guys, All the properties can now be overridden dynamically. Here is my latest example flow:
Although the info panel might need to be updated Currently all the properties are described under Properties but also under Dynamic properties because all properties can be overridden. Not sure if that is correct? On the other hand the Properties section contain the descriptions like in the config screen, while the Dynamic properties section contains the msg property names. So that is useful information, but every property description text is twice available in the info panel... |
BTW using the |
Why so? Are you no longer showing the notification on a new I'd interpreted the If a msg with a |
Ok then indeed no breaking change. |
The logic has been adapted, to show the notification when there is an |
I had forgotton to add documentation for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartbutenaers Thanks for improving the ui notification widget. Improvements are really helpful.
I found that there's a minor issue with the fill colour of the notification progress and added my comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gayanSandamal's approval is good enough for me here - let's get this in
Description
This PR adds some enhancements to the Notification ui node. Some of these enhancements were a bit too much related, so I decided to combine them in a single PR.
1. Help panel
There was no help panel yet for this node, so I added one:
2. Output msg fix
As described in issue 928 the output messages were not being send anymore. Don't know since which version, or which change caused it. Just fixed it...
3. Close via msg
As described in issue 733 it should be able to close the notification dialog using an input message.
I used some kind of toggle mechanism: when you inject a msg and the dialog is closed, then it will be opened. And the other way around:
So I don't look at the content of the payload. Don't know if that is ok? I have not mentioned it yet in the documentation for this reason.
4. Confirmation button
Beside the already existing dismiss button, there is now also a confirmation button. As described in issues 606 and (partially) 193.
By default the new confirmation button is inactive, to behave like older instances of this node:
In that case only the existing dismiss button is being displayed, similar to in the previous version:
When clicked, a message is send:
As discussed this is a breaking change because previously a payload "clicked" was being used, which would not be very useful in a context of two buttons...
When activated, it becomes possible to specify the label of the second button:
In this case the two buttons will be displayed:
When the new confirmation button is clicked, a message is send:
When only the new confirmation button is acivated:
Then only the new button is being displayed:
And when custom labels are specified for both buttons:
Then both labels will be displayed:
Related Issue(s)
Issue 928
Issue 733
Issue 606
Issue 193 (partially)
Checklist
flowforge.yml
?FlowFuse/helm
to update ConfigMap TemplateFlowFuse/CloudProject
to update values for Staging/ProductionLabels
area:migration
label