1990 remove css class on observable change #2327
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is meant to address Issue 1990.
New functionality for the CSS binding
With this pull request, the CSS binding will accept an array of strings which forms the definitive list of classes for the bound element. Below is my reasoning for this additional behaviour which can (in time) I believe replace the current use of class objects.
This functionality has been added to existing functionality for the sake of backwards compatibility.
Motivations
The CSS binding is inconsistent with other data bindings in the framework.
In its current form, I believe the CSS binding to be the only binding that requires both knowledge of the existing state and the necessary delta in order to reach a desired state. If you start with an element like:
<div data-bind="css: { myFirstClass: true, mySecondClass: true }"></div>
which generates
<div class="myFirstClass mySecondClass"></div>
and you desire to achieve the state:
<div class="myFirstClass myThirdClass"></div>
you are required to emit a change which contains the full delta from initial state to desired state:
<div data-bind="css: { myFirstClass: true, mySecondClass: false, myThirdClass: true }"></div>
Compare this to the text data binding where it is sufficient to emit only the desired state in order to make a change:
<p data-bind="text: 'This text will overwrite anything which was previously bound'"></p>
Presumably this is because the CSS binding was written to work in tandem with directly applied classes like so:
<div class="directly-applied-class" data-bind="css: { binding-applied-class: true }"></div>
However this is itself an inconsistency with other bindings, where directly applied content is overwritten by the data-binding:
<p data-bind="text: 'Text which will be shown'">Text which will be overwritten</p>
With the existing CSS binding functionality, if you do not emit a change for the delta to the desired state, the binding will only ever add classes without removing them. You can compare the behaviours with this pen based on existing functionality and this pen based on the change in this PR.
Knockout observableArrays are easier to manipulate than objects
Using arrays as the input for the CSS binding allows users to leverage the many helpers for array manipulation contained in knockout. For more on this, please refer to the documentation for working with knockout arrays.
Having the bound array be the definitive list of classes makes working with dynamic classes trivial
As raised in issue 1990 and evident from the discussion of pull 1710, working with dynamic classes in the current version of knockout isn't self explanatory. By making the CSS data-binding definitive for the list of classes on an element (as the text binding is for its contents) it becomes trivial to work with static and dynamic classes as shown by this pen using the changes proposed in this PR.