154 scheduled notifications allow users be able to add or delete the queries to use in their notification #170
Conversation
…ble-to-add-or-delete-the-queries-to-use-in-their-notification
Bundle size differenceComparing this PR to
|
…ble-to-add-or-delete-the-queries-to-use-in-their-notification
| @@ -108,7 +105,6 @@ export const DataTableComponent = <T extends RecordWithId>({ | |||
| display: 'flex', | |||
| flexDirection: 'column', | |||
| flex: 1, | |||
| height: '100%', | |||
There was a problem hiding this comment.
Removing this seems a bit dangerous, but doesn't seem to have had any ill effects that I've seen so far...
There was a problem hiding this comment.
🤔 I can't see why it was required
There was a problem hiding this comment.
🎉 Changes are working nicely - and it's all very good to use and the code looks great.
I have a couple of layout suggestions ( popped into a suggestion branch )
Main thing that troubles me is that all the bits are cramped on the form.. which is unrelated to this PR really..
Current
Suggested
I've also added the end of the breadcrumb - it was showing the ID but I think it's nicer with the title
One other nit pick when using the form.. I had trouble with the validation.
I thought that I had entered all the required fields - there was nothing else indicated in this view:
it turned out that the parameters were required - but because they aren't shown I didn't realise 🙄
Even opening the panel - they aren't indicated as required..
Then I caused a separate problem by deleting a parameter value, but the validation allows me to save still 🤔
| @@ -108,7 +105,6 @@ export const DataTableComponent = <T extends RecordWithId>({ | |||
| display: 'flex', | |||
| flexDirection: 'column', | |||
| flex: 1, | |||
| height: '100%', | |||
There was a problem hiding this comment.
🤔 I can't see why it was required
| height={modalHeight} | ||
| width={modalWidth} | ||
| okButton={ | ||
| <Tooltip title={t('label.select-queries')}> |
There was a problem hiding this comment.
Does the tooltip add anything? Given that it has the same text, I'm not convinced that it's useful.
If it is - perhaps it should be part of the button component itself
There was a problem hiding this comment.
I think it's a difference between DialogButton and LoadingButton
I'll change to dialog button ok variant like this?
I don't think it particularly important either way.
…layout-suggestion some ui suggestions
I think this one is kind of by design, and there is a comment in the code. There's a difference between a parameter with an empty string and an not filling in the field. I'm theorising that an empty string might be a valid option in some cases... |
I've put in a error state for missing parameters, doesn't automatically open the parameters tab though if you've closed it. I'm tempted to remove the close button as a simple fix, I also assume that only experienced people will be using the system so I think it's ok for now, and can be refined later if we get time. At least they'll get a red highlight now... |
Thanks heaps for the suggestions and the review! |
…ble-to-add-or-delete-the-queries-to-use-in-their-notification
jmbrunskill
deleted the
154-scheduled-notifications-allow-users-be-able-to-add-or-delete-the-queries-to-use-in-their-notification
branch
Closes #154
👩🏻💻 What does this PR do?
Gives user a method to see and select sql queries to use in templates.
Any required parameters are added to the params panel to be configured.
🧪 How has/should this change been tested?
Create SqlRecipientList (with params), Notification Query (with params)
Test different combinations of Scheduled notifications with different parameters, make sure the right combination of parameters shows in the parameter panel.
Check that Coldchain still works...
💌 Any notes for the reviewer?
This PR still doesn't do anything with the sql queries, that will be a later PR.