Proposal: Clean up `isEnabled` usage #638

Closed
publickeating opened this Issue Dec 11, 2011 · 9 comments

Projects

None yet

6 participants

@publickeating
Member

Description:

There is a pull request that is adding isEnabled support to SC.Button, but because there is already some isEnabled support within the framework, it got me wondering about how that Control and other Controls/Views should share isEnabled.

For example, the property is defined and there is support for adding/removing a disabled class to SC.View's but nothing for SC.TemplateView because the support is hardwired to SC.View in (core_foundation/views/view/enabled.js) as a reopen. SC.Control also depends on isEnabled, but its usage is flaky (only applies to a view with radio type inputs?). So there seems to be a nicer way that we can share the existing support between SC.View, SC.TemplateView and ??? any future view system.

Proposal:

I believe that disabling/enabling is a common enough paradigm that it should be a part of every View and therefore we should either make the reopen of SC.View in enabled.js to be a mixin so that we can mix it in to SC.View and SC.TemplateView and future view systems or that we put that code directly into SC.CoreView which should always be the root of any view system.

Also, that the special usage in SC.Control for radio inputs should be removed and left to whatever View has radio inputs.

Thoughts?

@charlesjolley
Member

I believe that disabling/enabling is a common enough paradigm that it should be a part of every View and therefore we should either make the reopen of SC.View in enabled.js to be a mixin so that we can mix it in to SC.View and SC.TemplateView and future view systems or that we put that code directly into SC.CoreView which should always be the root of any view system.
This makes sense to me. I would prefer to have features like this implemented as mixins rather than core extensions to the view class whenever possible. Views work best when they are made up of composable elements.
Also, that the special usage in SC.Control for radio inputs should be removed and left to whatever View has radio inputs.
+1

@mitchless
Contributor

I can't think of a view that would not benefit from generic enabling. The most straight-forward implementation would be on the base. Unless there is a performance benefit to a mixin implementation, SC.CoreView makes the most sense to me.

Sent from my iPad

On Dec 11, 2011, at 2:33 PM, Public Keatingreply@reply.github.com wrote:

Description:

There is a pull request that is adding isEnabled support to SC.Button, but because there is already some isEnabled support within the framework, it got me wondering about how that Control and other Controls/Views should share isEnabled.

For example, the property is defined and there is support for adding/removing a disabled class to SC.View's but nothing for SC.TemplateView because the support is hardwired to SC.View in (core_foundation/views/view/enabled.js) as a reopen. SC.Control also depends on isEnabled, but its usage is flaky (only applies to a view with radio type inputs?). So there seems to be a nicer way that we can share the existing support between SC.View, SC.TemplateView and ??? any future view system.

Proposal:

I believe that disabling/enabling is a common enough paradigm that it should be a part of every View and therefore we should either make the reopen of SC.View in enabled.js to be a mixin so that we can mix it in to SC.View and SC.TemplateView and future view systems or that we put that code directly into SC.CoreView which should always be the root of any view system.

Also, that the special usage in SC.Control for radio inputs should be removed and left to whatever View has radio inputs.

Thoughts?


Reply to this email directly or view it on GitHub:
#638

@geoffreyd
Contributor

From the docs of SC.Control:

A Control is a view that also implements some basic state functionality.
Apply this mixin to any view that you want to have standard control 
functionality including showing a selected state, enabled state, focus state, etc.

I personally have never seen a view that both: 1) needs to be enabled/disabled AND 2) is not a control (or similar)

I think that the isEnabled in SC.Control should just be extended to support more than just radio inputs. Then if a view wants to be able to be enabled/disabled, mixin SC.Control ... because that is now, probably, what it is. Most other cases, isVisible is what you want.

@mauritslamers
Member

Agree with geoffreyd: a disabled label (without edit support!) doesn't make sense to me.

@mitchless
Contributor

While it provides no functional benefit, it is common in UI toolkits
to have a visual distinction between "enabled" and "disabled" labels.
The label is typically "disabled" as part of a group of controls,
usually buddied with a data-entry control of some kind.

This "disabled" state provides additional visual cues to the user that
the control they are attempting to manipulate is disabled. This is
especially valuable with empty text fields, where the enabled and
disabled difference is either very subtle or indistinguishable.

On Tue, Dec 13, 2011 at 8:01 AM, Maurits Lamers
reply@reply.github.com
wrote:

Agree with geoffreyd: a disabled label (without edit support!) doesn't make sense to me.


Reply to this email directly or view it on GitHub:
#638 (comment)

@geoffreyd
Contributor

@mitchless that is true, good point.

My thought is, why we need to have a seperate mixin for 'enabled' when there is one (SC.Control) that is already designed (if not properly implemented) todo this? It seems to me that enabled.js was split out due to the whole "we're trying to make SC2" thing, and we don't really need to have it split out that much. I vote for pulling it back into SC.Control, and using that to control state of views (including labelViews).

Saying that I am open to other ideas, please discuss.

@mitchless
Contributor

I agree with @geoffreyd's point about the separate mixin; it does seem silly.

Is there a reason for "disabled" to be applied to SC.Control in lieu
of SC.CoreView?

On Tue, Dec 13, 2011 at 7:26 PM, Geoffrey Donaldson
reply@reply.github.com
wrote:

@mitchless that is true, good point.

My thought is, why we need to have a seperate mixin for 'enabled' when there is one (SC.Control) that is already designed (if not properly implemented) todo this? It seems to me that enabled.js was split out due to the whole "we're trying to make SC2" thing, and we don't really need to have it split out that much. I vote for pulling it back into SC.Control, and using that to control state of views (including labelViews).

Saying that I am open to other ideas, please discuss.


Reply to this email directly or view it on GitHub:
#638 (comment)

@publickeating
Member

I was actually thinking it was more of a general View behaviour, because the existing code is primarily visual only. But the comments got me thinking, if I wanted to "disable" a View, what would that mean? I believe it should mean that every childView of that view should be told to be disabled as well (and vice versa).

In that sense, I could tell my detail View to disable when the list View has no selection and all the labels, images and controls within that detail View would get isEnabled: NO. Therefore images could dim and lose their borders, labels could disappear, controls would not accept input, etc. Another example might be disabling an entire collection View or form View.

Thoughts?

@tim-evans
Member

+1, but should be configurable a lá cursors.

@publickeating publickeating added a commit that closed this issue Jun 7, 2013
@publickeating publickeating Reworks the isEnabled addition in order to allow for the enabled stat…
…e to actively cascade to child views. While you could previously use the `isEnabledInPane` property to determine if any ancestor view had been disabled, it was lazily computed and not bindable. With this change, `isEnabledInPane` is updated actively, which means that it can be used to update the child view's display if wanted. For example, a view gets the 'disabled' class if isEnabled is set to false, but it can also add isEnabledInPane as a displayProperty and use it to appear disabled if any ancestor becomes disabled.

This cascading can be blocked by any child view by setting `shouldInheritEnabled` to false, which allows you to set isEnabled on the top pane, but keep a section of child views separately enabled.

Includes unit tests.  Closes #638.
ded7fb5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment