unige-geohealth / accessmod

accessmod 5 : anisotropic accessibility analysis.
GNU Lesser General Public License v3.0
42 stars 14 forks source link

Referral analysis - Change the way the "Limit the spatial analysis to the closest pair time-wise" line gets inactivated when running the "Permute groups" setting #431

Closed SteeveEbener closed 8 months ago

SteeveEbener commented 9 months ago

Current Behavior

When you check the "Permute groups" setting, the "Limit the spatial analysis to the closest pair time-wise" line gets hidden as this analysis is not applicable in this case.

This being said, by being hidden you visually can't see the difference for this setting compared to running the analysis without permutation. In other words, by hiding it we don't know if it is checked or not unless we read and understand the small text attached to the "Permute groups" option.

Expected Behavior

The "Limit the spatial analysis to the closest pair time-wise" does not disappear when checking the "Permute groups" settings

Thanks

Possible Solution

Inactivate the possibility to check the box for the "Limit the spatial analysis to the closest pair time-wise" setting when the "Permute groups" setting is checked but not hide the line in question

nicolasray commented 9 months ago

OK we can do that. So this would mean: State of the box needs to be unchecked and not editable + the option needs to be greyed out so that the user sees that the option is not available.

SteeveEbener commented 9 months ago

Correct. Thanks

fxi commented 9 months ago

Something like this ?

https://github.com/unige-geohealth/accessmod/assets/1196833/65b8469d-8146-465b-b97f-d3d90a043c0c

SteeveEbener commented 9 months ago

Yes, except that the "Limit the spatial analysis to the closest pair time-wise" option should be unchecked when you check the "Permute groups" one

fxi commented 9 months ago

I understand your point. The actual logic is handled deeply in the code, so there will be no surprises. I tested both scenarios: altering the other option or just disable it, like in the video. It seemed wrong to have the mode selection change the state of the other option, as this can be unexpected and confusing for the user.

It's like having an eco mode in your electric car that also disables the AC. You can turn AC on or off before eco mode. When eco mode is off, you don't expect the AC to suddenly start at full blast, but to stay as it is was.

What I can try is add "Unused" in the checkbox label:

image
fxi commented 9 months ago

The behavior to ignore disabled inputs is also written in the html form submission specs https://html.spec.whatwg.org/

We don't use the submit mechanism, as it's a reactive app, but the behavior could be the same.

Available without the label change in 5.8.2-alpha.1.

SteeveEbener commented 9 months ago

Your analogy with the car AC does not apply here as you don't expect things to be as they were before but for the UI to be consistent and easily understandable.

I also doubt that most users know about the html submission specs, I don't.

When I see this: image

my reaction is that the "Limit..." setting is enabled and I can't change it.

Seing this:

image

is a bit better but still confusing (enabled but disabled)

The correct option according to me, to also be consistent with the text, is the first one you have tested, namely:

  1. When you enable the "Permute groups" setting, the "Limit..." one turns like this (independently from its current state, enabled or disabled); image
  2. If you disable the Permute groups" setting after that, then the "Limit..." one turns like this: image
fxi commented 9 months ago

I've implemented the feature as requested. We can monitor how it performs and make adjustments if needed. Available in 5.8.2-alpha.2 Thanks

SteeveEbener commented 9 months ago

Thanks

SteeveEbener commented 8 months ago

Working as expected. Thanks

Closing this issue