Fix: MCQ items assigned is-highlighted causing re-render (Issue/225)

[lemmyadams]

Addresses #225

Manually adds and removes the class for is-highlighted instead of through react.

[oliverfoster]

I don't understand what you're trying to do. But this is not the solution.

Manually modifying the DOM inside a React template is really not a good idea and we've spent a lot of time removing manual modifications for managed rendering.

Could you be more specific about what the "applied classes" are please?

[lemmyadams]

I don't understand what you're trying to do. But this is not the solution.

Manually modifying the DOM inside a React template is really not a good idea and we've spent a lot of time removing manual modifications for managed rendering.

Could you be more specific about what the "applied classes" are please?

The issue here is if you force a re-render which is what happens onItemFocus and onItemBlur, it removes any additional classes applied to the component. You said on the issue "It would be better if you added classes to items through the model.", to which I assumed you meant the view because the model in MCQ is basically empty, which is what I've done. If you add the class and remove the class, instead of updating the item properties which in turn adds/removes the class dynamically, you don't force a re-render.
An applied class could be something that changes the styling of the component to aid for accessibility

[oliverfoster]

An applied class could be something that changes the styling of the component to aid for accessibility

I need a better explanation of which classes are additionally applied to the component and which elements they are applied to.

It sounds as though classes are added to each item element, with some external code, and are subsequently removed upon rerender.

At the moment you've said "applied classes" and "component classes", which is incredibly non-specific and difficult to understand.

Note: Classes applied to the main component element are not managed by react and so wouldn't suffer the problem you're describing here, they would happen in the view, which is why I think you're adding classes to the items, but I need you to clarify please.

I don't understand the accessibility use-case for adding extra classes to an item, unless you're not adding classes to items, where it wouldn't be possible to use the existing classes on the item. We have classes for just about every state of the item, such as is-correct, is-selected, is-partly-correct etc etc, all of which can be chained together to style or select an mcq item in any number of possible configurations. If there is useful missing information from each of the items which isn't conveyed using a combination of the existing classes, it probably should be added as new class to the mcq code / template directly, proposed as a new feature.

If you add the class and remove the class, instead of updating the item properties which in turn adds/removes the class dynamically, you don't force a re-render.

Yes, there are a multitude of situations where react updates the dom, here - for only one of them - the code intentionally breaks the relationship between the model state and the dom, by not allowing react to manage the dom. The view here is not meant to be modifying the dom, that's the job of the react template, down this path is where we came from, manual dom updates in the view with jquery. This is the wrong direction.

I assumed you meant the view because the model in MCQ is basically empty

I did not mean the view. Please ask me if you don't understand something I've written, I'm not clear at the best of times.

The MCQ model is not empty, it inherits from ItemsQuestionModel which in turn inherits from ItemsComponentModel which has ItemModel as children. You can add a _classes attribute to the ItemModel and then have the mcq template render the _classes attribute on each item element. That way, adding classes can happen through the model and you can carry on utilising react to manage the dom, the view doesn't need to manually modify the dom and the relationship between the model state and the view state is maintained.

Using something like:

/**
 * Toggle a className in the _classes attribute of any Backbone.Model
 * @param model {Backbone.Model} Model with a _classes attribute to modify
 * @param className {string} Name of class to add/remove to _classes attribute
 * @param hasClass {boolean=true} true to add a class, false to remove
 */
function toggleModelClass(model, className, hasClass = true) {
  // split the string of classes into an array of classNames
  const currentClassNames = (model.get('_classes') || '').split(' ').filter(Boolean);
  // capture all existing classes as a unique list
  const classesObject = currentClassNames.reduce((classesObject , className) => ({ ...classesObject , [className]: true }), {})
  // update the list according to arguments
  classesObject[className] = hasClass
  // flatten back into a string of classes
  const outputClasses = Object
    .entries(classesObject)
    .filter(([, hasClass]) => hasClass)
    .map(([className]) => className)
    .join(' ')
  // update the model
  model.set('_classes', outputClasses)
}

const mcqModel = data.findById('<mcq model id>')
mcqModel.getChildren().forEach(child => {
  const isSelectedIncorrect = child.get('_isActive') && !child.get('_shouldBeSelected')
  toggleModelClass(child, 'is-selected-incorrectly', !isSelectedIncorrect)
});

The function toggleModelClass can effectively be used on any model with a _classes property.

[lemmyadams]

@oliverfoster how does MCQ trigger this function to fire though? Whenever the DOM updates we'd need to trigger this event to add or remove classes. This was the original approach I was taking which is outlined in the issue description itself

[oliverfoster]

The template + react adds classes to the DOM. The template reads the model which tells the template which classes to add. Updating the model causes the template to rerender. You should not add classes to the elements in the DOM directly, you should add classes to the model. You shouldn't need rendering events or to stop the render from happening if you just add them to the model and change the template to accommodate.

To which element(s) are you trying to add classes? Under which conditions are you trying to add classes? For what user information?

That function above is a helper to add classes to the _classes property on a model. You would call that function from wherever you need to add classes to the model. The template + react would then use the _classes attribute to add classes to the DOM element. The change of the _classes attribute on the model would cause the template + react to rerender.

Please explain which classes and elements you're talking about. I am utterly lost as to the elements and the classes. There are a lot of elements and classes in the mcq.