New Add-Ons Page

[shervElmi]

This PR updates and redesigns the Add-Ons page.

QA URLs:

https://qa.formidableforms.com/sherv13/wp-admin/admin.php?page=formidable-addons

Pro Version PR:

https://github.com/Strategy11/formidable-pro/pull/5168 Included in v6.11.2

Testing Instructions:

1. Ensure everything works on the new Add-Ons page as shown in the screencast: https://share.cleanshot.com/DFDW4jdQ

2. This PR has refactored the "Form Templates" and "Onboarding Wizard" pages, so make sure they work as before.

[Crabcyborg]

@shervElmi I updated my e2e test fixes to get Cypress passing, and merged those changes into this PR #2012

I think this is good to go now.

One thing I noticed though, is that I went to turn off a plugin, then left the page too soon so the request never completed and my plugin was still enabled. Is it possible to add an exit confirmation if an install is currently in progress?

You can trigger a pop-up using the beforeunload event using window.addEventListener( 'beforeunload', maybeConfirmExit );. I copied that line from style.js.

The maybeConfirmExit function is pretty basic. The confirmation pop-up text is impossible to customize, so we can just do the same thing here (with a check for something like makingRequest instead of unsavedChanges).

/**
 * Possibly prevent leaving the page if there are unsaved changes.
 *
 * @param {Event} event
 * @returns {void}
 */
function maybeConfirmExit( event ) {
	if ( ! state.unsavedChanges ) {
		return;
	}

	event.preventDefault();
	event.returnValue = '';
}

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (6)

tests/cypress/e2e/admin-a11y.cy.js (1)

Line range hint 1-150: Consider a more proactive approach to accessibility testing.

While the changes in this file allow the Cypress tests to pass by ignoring specific accessibility rules, they don't address the underlying accessibility issues in the components themselves. This approach might lead to a false sense of accessibility compliance.

Instead of disabling accessibility rules in tests, consider the following suggestions:

1. Review the components flagged by these rules (Add-Ons and SMTP pages) and refactor them to meet accessibility standards where possible.
2. If certain accessibility rules cannot be met due to specific design or functional requirements, document these exceptions clearly in the code or in a separate accessibility statement.
3. Implement a process for regularly reviewing and addressing accessibility issues as part of the development workflow.

By addressing accessibility at the component level rather than in the tests, you can ensure a more inclusive user experience and maintain genuine accessibility compliance.

Would you like assistance in developing a strategy for improving the overall accessibility of the application?

js/admin/style.js (5)

Line range hint 1-624: Consider improving code consistency and modernization

The codebase mixes jQuery and vanilla JavaScript. While this is not inherently problematic, it can lead to inconsistencies and make maintenance more challenging. Consider gradually migrating towards vanilla JavaScript for better performance and to reduce dependencies.

Examples of mixed usage:

1. Line 14: jQuery( 'input.hex' ).wpColorPicker({
2. Line 185: const { debounce } = frmDom.util;

Consider refactoring jQuery-dependent code to use vanilla JavaScript where possible. For instance:

// Instead of:
jQuery( 'input.hex' ).wpColorPicker({
  // ...
});

// Consider:
document.querySelectorAll('input.hex').forEach(input => {
  // Use a vanilla JS color picker library or custom implementation
});

This transition can be done gradually to maintain compatibility with existing jQuery plugins while moving towards a more modern, lightweight codebase.

🧰 Tools

🪛 Biome

[error] 7-7: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

---

Line range hint 1-624: Optimize event handling with event delegation

The code attaches multiple event listeners to individual elements, which can be inefficient for performance, especially with a large number of elements. Consider using event delegation to attach fewer event listeners and improve performance.

Implement event delegation for repeated event listeners. For example:

// Instead of:
document.querySelectorAll('.styling_settings h3.accordion-section-title').forEach(el => {
  el.addEventListener('click', event => {
    maybeCollapseSettings(event);
  });
  // ...
});

// Consider:
document.querySelector('.styling_settings').addEventListener('click', event => {
  if (event.target.matches('h3.accordion-section-title')) {
    maybeCollapseSettings(event);
  }
});

This approach reduces the number of event listeners and improves performance, especially for dynamically added elements.

🧰 Tools

🪛 Biome

[error] 7-7: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

---

Line range hint 1-624: Address TODO comments and remove unused code

There are some TODO comments and potentially unused code sections in the file. Addressing these can improve code clarity and maintainability.

1. Review and address the TODO comment on line 4:

   // TODO: Stop triggering change events with jQuery. And remove the other jQuery as well.

2. Remove or implement any commented-out code if it's no longer needed.

Cleaning up these items will improve code readability and reduce confusion for future maintenance.

🧰 Tools

🪛 Biome

[error] 7-7: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

---

Line range hint 1-624: Improve code organization and modularity

The file contains a large number of functions and event listeners, which can make it difficult to understand and maintain. Consider reorganizing the code into smaller, more focused modules or classes.

Refactor the code to improve modularity:

1. Group related functions into classes or modules. For example, create a StylePreview class to handle all preview-related functionality.
2. Use ES6 modules to split the code into separate files based on functionality.
3. Implement a main initialization function that sets up all necessary event listeners and initializes required modules.

Example structure:

// stylePreview.js
export class StylePreview {
  constructor() {
    // ...
  }

  initPreview() {
    // ...
  }

  changeStyling() {
    // ...
  }
}

// main.js
import { StylePreview } from './stylePreview.js';

function initStyler() {
  const preview = new StylePreview();
  preview.initPreview();
  // ...
}

document.addEventListener('DOMContentLoaded', initStyler);

This approach will make the code more maintainable, testable, and easier to understand.

🧰 Tools

🪛 Biome

[error] 7-7: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

---

Line range hint 1-624: Summary and Recommendations

The changes made to the file, particularly the early return for missing elements, are positive improvements that enhance the script's robustness. However, there are several areas where the code could be further improved:

1. Gradually migrate from jQuery to vanilla JavaScript for better performance and reduced dependencies.
2. Implement event delegation to optimize event handling and improve performance.
3. Address TODO comments and remove unused code to improve clarity and maintainability.
4. Refactor the code to improve modularity, possibly by using ES6 modules and organizing related functionality into classes or separate files.

These improvements will lead to a more maintainable, performant, and modern codebase. It's recommended to implement these changes incrementally to ensure stability and ease of testing.

Consider creating a roadmap for implementing these suggestions in future iterations of the codebase.

🧰 Tools

🪛 Biome

[error] 7-7: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c1d7b8b and f19aa9f.

📒 Files selected for processing (4)

• classes/views/form-templates/list.php (3 hunks)
• js/admin/style.js (1 hunks)
• tests/cypress/e2e/Form Templates/FormTemplates.cy.js (6 hunks)
• tests/cypress/e2e/admin-a11y.cy.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• classes/views/form-templates/list.php
• tests/cypress/e2e/Form Templates/FormTemplates.cy.js

🔇 Additional comments (3)

tests/cypress/e2e/admin-a11y.cy.js (2)

123-124: Review the landmark structure on the SMTP page.

The addition of { id: 'landmark-complementary-is-top-level', enabled: false } to the ignored rules for the SMTP page accessibility test indicates that there might be nested complementary landmarks on this page.

While ignoring this rule allows the tests to pass, it's important to consider the impact on accessibility. Proper landmark structure helps screen reader users navigate and understand the page layout more effectively.

Could you provide more context on why this rule needed to be disabled? Is there an opportunity to refactor the SMTP page to ensure that complementary landmarks are at the top level, thus improving its accessibility?

#!/bin/bash
# Description: Check for landmark structure in the SMTP page component

# Test: Search for elements with role="complementary" in the SMTP page component
ast-grep --lang javascript --pattern 'role="complementary"' | rg -A 5 'SMTP'

---

100-101: Consider reviewing the heading structure on the Add-Ons page.

The addition of { id: 'heading-order', enabled: false } to the ignored rules for the Add-Ons page accessibility test suggests that there might be a non-standard heading structure on this page.

While ignoring this rule allows the tests to pass, it's worth investigating why the heading order doesn't meet the standard accessibility guidelines. Proper heading structure is crucial for screen reader users to navigate and understand the page content effectively.

Could you provide more context on why this rule needed to be disabled? Is there an opportunity to refactor the Add-Ons page to improve its heading structure and meet accessibility standards?

js/admin/style.js (1)

9-11: Approved: Early return for missing element

This addition is a good defensive programming practice. It prevents the script from executing further if the required element #frm_active_style_form is not present in the DOM. This can help avoid potential errors and unnecessary code execution, improving the script's robustness.

[Crabcyborg]

I'll go ahead and merge this. Adding an exit confirmation can be done in a new PR.

🚀