Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential React and jQuery conflict when removing component views #508

Open
danielghost opened this issue Apr 11, 2024 · 3 comments
Open

Comments

@danielghost
Copy link

danielghost commented Apr 11, 2024

Subject of the issue/enhancement/features

If the _vanilla theme object is applied to a component model, a background div will be added to the component (regardless of whether _backgroundImage is configured). If applied to a component using React templates, this produces the following error when attempting to remove the view:
react-dom.development.js:61 Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by another copy of React.

The theme object is applied as default when using the AAT.

This is presumably because jQuery is being used to manipulate the DOM since 1a8a9b6, causing an issue with React when it attempts to unmount.

As this only currently occurs for component because other views are still using Handlebars, it will have been introduced with the themeComponentView addition in a13e2f0#diff-84cd82bc77528fe71fd5802fc8120448ee0b922be0ff695a14a9bd29751033df. Previously the theme wasn't applied to component views.

The error doesn't occur if the background is removed in onRemove(), before AdaptView.remove calls unmountComponentAtNode.

The error also only occurs when compiling via grunt dev. grunt build; rub dev; rub build; don't produce the error.

First thing to consider before a fix is applied, is whether a component view should even apply a background? It isn't part of https://github.com/adaptlearning/adapt-contrib-vanilla/blob/master/schema/component.schema.json and has-bg-image class is not configured in https://github.com/adaptlearning/adapt-contrib-vanilla/blob/master/less/core/component.less as per page, article and block, so currently this won't get displayed anyway.

Your environment

  • since 5.22.11
@oliverfoster
Copy link
Member

oliverfoster commented Apr 11, 2024

At some point all of the views will be react - I don't think exclusion from components provides a good line of resolution.

I would prefer us to standardise the background layer so that there is place to inject it on all containers.

It's proven weirdly useful for all kinds of decorative, non-accessible media and effects, especially as an alternative to the before/after CSS content spaces we used to create.

@swashbuck
Copy link
Contributor

Related: #511

@swashbuck
Copy link
Contributor

@danielghost Component backgrounds are definitely being used. I'm providing a Less fix here: #512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants