Port forward: ui fixes and better error handling #56
Conversation
kevincela
commented
Oct 11, 2023
kevincela
commented
- add error handling for fetching firewall config inside CreateOrEditPortForwardDrawer
- move destination zone combobox inside advanced settings in CreateOrEditPortForwardDrawer
- replace empty cards in PortForward page with NeEmptyState
- clear errors when exiting CreateOrEditPortForwardDrawer
- move destination zone combobox inside of advanced settings
- handle firewall config error in port forward drawer - avoid fetching firewall config if data is already loaded
- clear error on close only on add/edit request errors
| @@ -165,13 +165,25 @@ watchEffect(() => { | |||
| }) | |||
|
|
|||
| function close() { | |||
| if ( | |||
There was a problem hiding this comment.
Is this condition needed?
There was a problem hiding this comment.
Not strictly needed, but if the error is given by a failed request of the form's options then clearing it could be confusing if the user reopens it again (they would see no error notification, but the options still aren't available), whereas for edit/create request errors it makes sense because the item gets cleared on exit. Maybe at this point we should simply refetch the options every time the drawer is opened, in case of an error?
There was a problem hiding this comment.
Yes, if the drawer need some data we usually fetch it every time it opens, so we can get updated data. If the code doesn't currently work this way we can postpone these changes to another PR, it's not an essential feature at the moment.
There was a problem hiding this comment.
Ok, I've updated the drawer so that it refetches the data every time it is shown, now it should be ok. I've also removed the aforementioned condition because of this change.
| class="mb-6" | ||
| kind="error" | ||
| /> | ||
| <NeSkeleton v-if="loading || firewallConfig.loading" :lines="10" /> | ||
| <NeSkeleton v-if="loading || firewallConfig.loading || firewallConfig.error" :lines="10" /> |
There was a problem hiding this comment.
We usually don't show a skeleton if an error is displayed
There was a problem hiding this comment.
Hmm, this is true but doing it this way could also cause a scenario where the user can interact with the form even if the firewallConfig fetch did not go as planned... How should we handle this case?
There was a problem hiding this comment.
Hmm, this is true but doing it this way could also cause a scenario where the user can interact with the form even if the firewallConfig fetch did not go as planned... How should we handle this case?
If an error occurs while retrieving drawer data we usually display only the error and hide the form
There was a problem hiding this comment.
Ok, now if there's an error while fetching the firewall config the form does not show. I've also removed the condition from the skeleton.
| resetForm() | ||
| emit('close') | ||
| } | ||
|
|
||
| onMounted(() => { | ||
| fetchOptions() | ||
| firewallConfig.fetch() | ||
| if (firewallConfig.loading || firewallConfig.error) { |
There was a problem hiding this comment.
Can you explain this condition?
There was a problem hiding this comment.
Considering how useFirewallStore is a global store and is used in different pages other than the port forward one, I thought it wouldn't make much sense to refetch its items when they already loaded, hence why i've included this condition. If you think this may cause issues I can remove it though!
There was a problem hiding this comment.
Considering how
useFirewallStoreis a global store and is used in different pages other than the port forward one, I thought it wouldn't make much sense to refetch its items when they already loaded, hence why i've included this condition. If you think this may cause issues I can remove it though!
Now it's clear, thanks
- refetch items every time the drawer is shown, instead of onmounted - clear errors every time the drawer is closed - remove firewallconfig fetch error from skeleton - avoid showing form if there is a firewallconfig fetch error
