Disable submit button while fetching selected revision. #1810
Conversation
...and improve corresponding CI test to ensure we click only when enabled. It seems that puppeteer will click on a button whether or not it is enabled (which perhaps makes sense), hence the requirement to explicitly select the not-disabled button.
| @@ -28,6 +28,7 @@ function mapStateToProps( | |||
| reposIsFetching: repos.isFetching, | |||
| appsError: apps.error, | |||
| chartsError: charts.selected.error, | |||
| disabled: charts.isFetching, | |||
There was a problem hiding this comment.
We should forward the isFetching property instead and not render the form if it's fetching. The problem is that if the new chart version is still being fetched and the user changes something, the result may be inconsistent. For example, the new changes from the selected version won't be applied.
There was a problem hiding this comment.
We should forward the
isFetchingproperty instead
Hmm - I can do that, but I thought the point of the container / component separation is that the container is meant to know about behaviour like whether things are being fetched, while the component should be dumb and just know whether it is enabled or disabled. But happy to switch it.
and not render the form if it's fetching.
Though that would be fine when displaying the form initially, it'd be a very jerky flash every time you change the revision, no? (currently the form is just updated with any new fields etc.) Again, I'm OK doing this, just not sure it's the best option - I would have thought what we want to do is just ensure the form cannot be submitted (mainly for CI), but even nicer if the whole form is disabled. I don't see why we'd want to flash to the loading and back.
Yeah - what I wanted to do was to disable the form, but disabled applies to a bunch of elements, as well as field-sets... so the best we could do is use a fieldset and disable that.
The problem is that if the new chart version is still being fetched and the user changes something, the result may be inconsistent.
It's pretty hard to do so - remember this is really fixing a CI issue where the runner is too fast (inhumanly fast) and submits the form (with changes) before the chart version has loaded. But I do agree it'd be better (see above, I'd wanted to disable the whole form).
There was a problem hiding this comment.
Hmm - I can do that, but I thought the point of the container / component separation is that the container is meant to know about behaviour like whether things are being fetched, while the component should be dumb and just know whether it is enabled or disabled. But happy to switch it.
Well, the container is just connecting properties and functions to the component (there should be no logic in the container, at least that's the convention we are following). It's true that the dumber the component is the better but for the isFetching property, that's something we usually forward because the component needs to render the loading wrapper in that case.
Though that would be fine when displaying the form initially, it'd be a very jerky flash every time you change the revision, no? (currently the form is just updated with any new fields etc.) Again, I'm OK doing this, just not sure it's the best option - I would have thought what we want to do is just ensure the form cannot be submitted (mainly for CI), but even nicer if the whole form is disabled. I don't see why we'd want to flash to the loading and back.
Yeah - what I wanted to do was to disable the form, but
disabledapplies to a bunch of elements, as well as field-sets... so the best we could do is use a fieldset and disable that.
yes, the intermittent loading wrapper may be a bit annoying but better that than having unexpected changes because the resources are still loading. Maybe it's better as you say to disable the whole form but that may be more complex, also it could be confusing for the user if nothing is showing that we are loading stuff.
The problem is that if the new chart version is still being fetched and the user changes something, the result may be inconsistent.
It's pretty hard to do so - remember this is really fixing a CI issue where the runner is too fast (inhumanly fast) and submits the form (with changes) before the chart version has loaded. But I do agree it'd be better (see above, I'd wanted to disable the whole form).
yes, it's not a problem for us but maybe in some environments (with a remote db or something like that) it could be slow enough.
There was a problem hiding this comment.
Well, the container is just connecting properties and functions to the component (there should be no logic in the container, at least that's the convention we are following).
You're right - I was basing my thoughts on outdated info, it seems - still thinking about the (container/component separation between functionality and presentation, but as per the update on the original post, things have moved on to hooks, with good reason.
In particular, hooks allow you to access (redux) state within component functions, so we'd no longer need to pass state all the way down (wow, that would be great). But more relevant for this PR, the component selects some state from redux, which would mean it would indeed just be the charts.isFetching or repos.isFetching etc.
So I've updated to that as it'll make it simpler for us to transition to hooks (and we should start removing container/component stuff as we work through the codebase, I reckon? Thoughts?)
yes, the intermittent loading wrapper may be a bit annoying but better that than having unexpected changes because the resources are still loading. Maybe it's better as you say to disable the whole form but that may be more complex, also it could be confusing for the user if nothing is showing that we are loading stuff.
Yep, updated. The flash is a bit annoying - especially since the selector itself disappears temporarily, but OK.
As a result, the CI script no longer needs to explicitly look for a non-disabled submit button.
Updated the prop to `formIsReady` rather than independent fooIsFetching knowledge.
| @@ -24,11 +24,10 @@ function mapStateToProps( | |||
| ) { | |||
| return { | |||
| app: apps.selected, | |||
| appsIsFetching: apps.isFetching, | |||
| formIsReady: (!apps.isFetching) && (!charts.isFetching), | |||
There was a problem hiding this comment.
in other places we simply call this isFetching but it's okay
There was a problem hiding this comment.
That must have been an outdated diff you were looking at? (I'd originally collapsed them into formIsReady but then after reading about moving to hooks instead, it made more sense to use chartsIsFetching and leave appsIsFetching as is - so updated to that. Somehow you've got the intermediate diff (from c73221a ) rather than the final diff. Ah - perhaps from email?
There was a problem hiding this comment.
no no, I meant that in other components, this property would be usually named isFetching (instead of adapting the name for each component).
...and improve corresponding CI test to ensure we click only when
enabled.
It seems that puppeteer will click on a button whether or not it is
enabled (which perhaps makes sense), hence the requirement to explicitly
select the not-disabled button.
I'm 5 from 5 running this locally - let's see if that's consistent with CI.