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

Fix usage of requiresReindex #2536

Merged
merged 3 commits into from
Dec 20, 2021
Merged

Fix usage of requiresReindex #2536

merged 3 commits into from
Dec 20, 2021

Conversation

felipeelia
Copy link
Member

Description of the Change

In #2491 we've introduced a new confirmation prompt on features activation. This PR adjusts its implementation so the prompt only appears in features that really require a reindex.

Changelog Entry

Append to the Enabling features that require a reindex will now ask for confirmation line

@@ -605,7 +606,7 @@ $features.on('change', '.js-toggle-feature', function (event) {
const { value } = event.target;
const { requiresReindex, wasActive } = event.currentTarget.dataset;

if (requiresReindex && wasActive === '0' && value === '1') {
if (requiresReindex === '1' && wasActive === '0' && value === '1') {
container.style.display = 'block';
} else {
container.style.display = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipeelia we need to check if there is an element on container because we are retrieving only if it has .requirements-status-notice--reindex and it is causing an error when we try to access container.style.display:

Cannot read properties of null (reading 'style')

Maybe we can add an early return:

if (!container) return;

@felipeelia felipeelia merged commit 1d71ce3 into develop Dec 20, 2021
@felipeelia felipeelia deleted the fix/feature-reindex-alert branch December 20, 2021 17:20
felipeelia added a commit that referenced this pull request Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants