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

🧪 ⚗️ 🚧 Proof of concept to abstract the rules logic 🚧 #429

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

aristath
Copy link
Collaborator

This PR is just a prototype to investigate how we could abstract the logic for rules in a more object-oriented way.

Please note that there are some modifications where we include the files and run the evaluations to allow running both the old and new implementations for the sake of this experiment/POC, showcasing how we could do it. I'm also hard-including the new classes because I'm treating this as a rough proof of concept to demonstrate an idea 😅

What is your opinion on doing something like this @boonedev @jdevalk @SteveJonesDev ?

Right now we have some basic data for rules in the includes/rules.php file, and then we have the rule's evaluation in includes/rules/{RULE_SLUG}.php files. This PR attempts to do the following:

  • Adds a Rules object, which can act as a collection of rules.
  • Adds an abstract Rule object, which individual rules can extend
  • Adds a Rule_Aria_Hidden as a demostration of how a rule can be implemented, extending the Rule object. This class unifies the rule info from includes/rules.php and includes/rules/aria_hidden.php
  • Deletes the includes/rules/aria_hidden.php file since the info is now in the Rule_Aria_Hidden object, and removes the rule from includes/rules.php

Comment on lines 132 to 155
/**
* Include Rules
*/
require_once plugin_dir_path( __FILE__ ) . 'includes/classes/class-rules.php';
require_once plugin_dir_path( __FILE__ ) . 'includes/classes/rules/class-rule.php';
require_once plugin_dir_path( __FILE__ ) . 'includes/classes/rules/class-rule-aria-hidden.php';
( new EDAC\Rules() )->add_rule( 'aria_hidden', new EDAC\Rules\Rule_Aria_Hidden() );
add_filter(
'edac_filter_register_rules',
function ( $rules ) {
$rules_from_objects = ( new EDAC\Rules() )->get_rules();
foreach ( $rules_from_objects as $rule ) {
$rules[] = array(
'title' => $rule->get_title(),
'info_url' => $rule->get_info_url(),
'slug' => $rule->get_slug(),
'rule_type' => $rule->get_type(),
'summary' => $rule->get_summary(),
);
}
return $rules;
}
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just for the POC, to show how we can add rules in a manner that is backwards-compatible.

Comment on lines +260 to +263
$rule_file_path = plugin_dir_path( __FILE__ ) . 'includes/rules/' . $rule['slug'] . '.php';
if ( ! file_exists( $rule_file_path ) ) {
continue;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was done simply because I removed the aria_hidden rule file

@amberhinds
Copy link
Member

@aristath have you looked at how the rules are changing with the transition to JavaScript scanning? https://github.com/equalizedigital/accessibility-checker/tree/feature/javascript-scan

@aristath
Copy link
Collaborator Author

@amberhinds thank you, I had not seen that!
So... if we do JS checks, what's the plan for checks running in the editor? 🤔

@boonedev
Copy link
Contributor

Hi @aristath, thanks for the code cleanup help! Here's an overview of how checks are running in the editor:
https://github.com/equalizedigital/accessibility-checker-pro/blob/develop/README_DEV.txt

@SteveJonesDev
Copy link
Member

@aristath, see the readme @boonedev posted for details. Currently, the only rule converted to Javascript is the color contrast as it was the rule causing the most false positives. We'll have a hybrid approach until all rules are converted over using either axe-core or a custom Javascript rule. So we'll still need to refactor the rules logic as you've demonstrated. Unless we push forward and convert all the rules now.

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

Successfully merging this pull request may close these issues.

4 participants