Get cypress passing, improve slidein accessibility#2056
Conversation
…top including formidable-forms file path in URL checks
…t value is not a match, add a role to the slidein to improve accessibility
WalkthroughThe pull request introduces several updates across multiple files. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (1)
Line range hint
1-357: Summary: Image path updates and format consistencyThe changes in this file primarily involve updating image source paths for various application templates. The new paths follow a consistent pattern: '/images/applications/thumbnails/{template-name}.{extension}'. This update improves the organization and clarity of the image references in the tests.
However, there's an inconsistency in the image formats used across different templates. Some use .png while others use .webp. It's recommended to review this inconsistency and consider standardizing the image format across all templates, unless there's a specific reason for using different formats.
Action items:
- Verify that all updated image paths are correct and the images exist in the specified locations.
- Review the use of .png vs .webp formats and consider standardizing if possible.
- Update any documentation or guidelines related to adding new application templates to reflect the new image path structure and preferred format.
classes/controllers/FrmAppController.php (3)
Line range hint
1362-1395: LGTM! Consider adding version comparison for Pro compatibility.The
enqueue_floating_linksmethod is well-implemented, following WordPress best practices for enqueueing scripts and styles. It includes proper checks, internationalization support, and a hook for Pro version compatibility.Consider adding a version comparison check for the Pro compatibility hook:
if ( defined('FRM_PRO_VERSION') && version_compare( FRM_PRO_VERSION, '5.0', '>=' ) ) { do_action( 'frm_enqueue_floating_links' ); }This ensures compatibility with older Pro versions that might not support this hook.
Line range hint
1397-1453: Effective, but consider using WordPress filters for better compatibility.The
filter_admin_noticesmethod successfully removes third-party admin notices on Formidable pages while preserving Formidable's own notices. However, directly modifying the$wp_filterglobal can potentially lead to conflicts or unexpected behavior.Consider using WordPress's
remove_all_actions()function instead of directly modifying$wp_filter. This approach is more aligned with WordPress standards and less likely to cause conflicts:public static function filter_admin_notices() { if ( ! self::in_our_pages() ) { return; } $actions = array( 'admin_notices', 'network_admin_notices', 'user_admin_notices', 'all_admin_notices', ); foreach ( $actions as $action ) { remove_all_actions( $action ); add_action( $action, array( __CLASS__, 'show_formidable_notices' ) ); } } public static function show_formidable_notices() { do_action( 'frm_admin_notices' ); }This approach removes all notices and then adds back only Formidable notices through a custom action.
Line range hint
1455-1470: LGTM! Consider adding security checks and making the method more flexible.The
redirect_blackfridaymethod effectively redirects users to the Black Friday sales page. It's good that it usesFrmAppHelper::admin_upgrade_linkfor URL generation and tracking.Consider the following improvements:
Add a nonce check for security:
check_admin_referer( 'frm_black_friday_redirect' );Make the method more flexible by accepting parameters:
public static function redirect_blackfriday( $campaign = 'black-friday', $medium = 'black-friday-submenu' ) { check_admin_referer( 'frm_black_friday_redirect' ); $content = self::is_cyber_monday() ? 'cyber-monday-submenu' : $campaign . '-submenu'; wp_redirect( FrmAppHelper::admin_upgrade_link( array( 'medium' => $medium, 'content' => $content, ), $campaign ) ); die(); }This allows for easier reuse and testing of the method with different campaigns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- classes/controllers/FrmAppController.php (1 hunks)
- classes/controllers/FrmOnboardingWizardController.php (0 hunks)
- cypress.config.js (1 hunks)
- js/packages/floating-links/s11-floating-links.js (1 hunks)
- tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (21 hunks)
💤 Files with no reviewable changes (1)
- classes/controllers/FrmOnboardingWizardController.php
🧰 Additional context used
🔇 Additional comments (19)
cypress.config.js (1)
18-18: Verify the impact of enabling experimentalRunAllSpecsThe addition of
experimentalRunAllSpecs: trueenables an experimental Cypress feature to run all specs in a single execution. While this can potentially speed up overall test execution, it's important to consider the following:
- As an experimental feature, it may introduce unexpected behavior or instability.
- Running all specs at once could impact performance and resource usage, especially for larger test suites.
- This change might affect how developers run and debug tests locally.
- It could alter the execution pattern in CI/CD pipelines.
To ensure this change doesn't negatively impact your testing process:
Run the following command to check if there are any warnings or deprecation notices related to this feature:
Compare the execution time and resource usage:
Check for any changes in test behavior or stability:
If these checks pass without issues, the change can be considered safe to merge.
js/packages/floating-links/s11-floating-links.js (1)
116-116: Excellent accessibility improvement!Adding the 'alert' role to the slide-in element enhances the accessibility of the component. This change ensures that users relying on assistive technologies will be immediately notified of important, time-sensitive information when the slide-in appears.
This modification:
- Improves the user experience for those using screen readers.
- Enhances compliance with accessibility guidelines.
- Provides clearer semantic meaning to the element's purpose.
Great job on prioritizing accessibility in your code!
tests/cypress/e2e/Applications/validateApplicationsPage.cy.js (16)
42-42: LGTM: Updated image path for Business Directory templateThe image source path has been updated to a more concise and consistent format. This change aligns with the overall update to image paths in the test file.
54-54: LGTM: Updated placeholder image pathThe placeholder image path has been updated to align with the new directory structure. This change is consistent with the overall update to image paths in the test file.
87-87: LGTM: Updated image path for Business Hours templateThe image source path for the Business Hours template has been updated to follow the new consistent pattern. This change aligns with the overall update to image paths in the test file.
111-111: LGTM: Updated image path for Charity Tracker templateThe image source path for the Charity Tracker template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with the Certificate template.
123-123: LGTM: Updated image path for Contract Agreement templateThe image source path for the Contract Agreement template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with the recent template updates.
147-147: LGTM: Updated image path for Freelance Invoice Generator templateThe image source path for the Freelance Invoice Generator template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with most recent template updates.
159-159: LGTM: Updated image path for Invoice PDF templateThe image source path for the Invoice PDF template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with most recent template updates.
170-170: LGTM: Updated image path for Letter of Recommendation templateThe image source path for the Letter of Recommendation template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with most recent template updates.
182-182: LGTM: Consistent update to modal placeholder image pathThe image source path for the modal placeholder has been updated to '/images/applications/placeholder.png'. This change is consistent with the previous update to the modal placeholder image path (line 54), maintaining consistency across all modal placeholder image assertions.
215-215: LGTM: Updated image path for Link in Bio Instagram Page templateThe image source path for the Link in Bio Instagram Page template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with most recent template updates.
227-227: LGTM: Updated image path for Member Directory templateThe image source path for the Member Directory template has been updated to follow the new consistent pattern. The use of the .webp format is consistent with most recent template updates.
251-251: LGTM: Updated image path for Real Estate Listing templateThe image source path for the Real Estate Listing template has been updated to follow the new consistent pattern. The use of the .png format is consistent with the previous template (Product Review and Purchase). However, it's worth noting that this differs from the .webp format used in most other recent template updates. Ensure that this is intentional and aligns with the overall image format strategy.
239-239: LGTM: Updated image path for Product Review and Purchase templateThe image source path for the Product Review and Purchase template has been updated to follow the new consistent pattern. However, note that this image uses the .png format, which differs from the .webp format used in most recent template updates. Ensure that this difference is intentional and consistent with the overall image format strategy.
#!/bin/bash # Verify that the Product Review and Purchase template image exists and is in .png format file_path="/images/applications/thumbnails/product-review.png" if [ -f "$file_path" ]; then file_type=$(file -b --mime-type "$file_path") if [ "$file_type" == "image/png" ]; then echo "Product Review and Purchase template image exists and is in .png format" else echo "Product Review and Purchase template image exists but is not in .png format" fi else echo "Product Review and Purchase template image does not exist at the specified path" fi
263-263: LGTM: Updated image paths for multiple templatesThe image source paths for Restaurant Menu, Team Directory, Testimonials, Business Directory, and Business Hours templates have been updated to follow the new consistent pattern. The modal placeholder image path has also been updated consistently.
However, there's an inconsistency in the image formats used:
- Restaurant Menu, Team Directory, Business Directory, and Business Hours use .png
- Testimonials uses .webp
Consider standardizing the image format across all templates for consistency, unless there's a specific reason for using different formats.
#!/bin/bash # Check for consistency in image formats across all templates echo "Image format distribution in /images/applications/thumbnails/:" find /images/applications/thumbnails -type f | sed 's/.*\.//' | sort | uniq -c | sort -rn # List any templates that don't have both .png and .webp versions echo "\nTemplates missing either .png or .webp version:" for template in $(find /images/applications/thumbnails -type f | sed 's/\.[^.]*$//' | sort | uniq); do png_exists=$(find "${template}.png" -maxdepth 0 -type f 2>/dev/null) webp_exists=$(find "${template}.webp" -maxdepth 0 -type f 2>/dev/null) if [ -z "$png_exists" ] || [ -z "$webp_exists" ]; then echo "$template" fi doneAlso applies to: 275-275, 286-286, 299-299, 341-341, 351-351
135-135: LGTM: Updated image path for FAQ templateThe image source path for the FAQ template has been updated to follow the new consistent pattern. However, there are two points to note:
- This image uses the .png format, which differs from the .webp format used in recent template updates.
- The filename includes "wordpress", which is unique compared to other templates.
Ensure that these differences are intentional and consistent with the overall image naming and format strategy.
✅ Verification successful
Verified: Image path and format are correct
- The FAQ template image exists at
images/applications/thumbnails/faq-template-wordpress.pngand is in.pngformat.- The thumbnails directory contains 7
.pngfiles and 9.webpfiles, maintaining consistency in image formats.No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the FAQ template image exists and check its format file_path="/images/applications/thumbnails/faq-template-wordpress.png" if [ -f "$file_path" ]; then file_type=$(file -b --mime-type "$file_path") if [ "$file_type" == "image/png" ]; then echo "FAQ template image exists and is in .png format" else echo "FAQ template image exists but is not in .png format" fi else echo "FAQ template image does not exist at the specified path" fi # Check for consistency in image formats png_count=$(find /images/applications/thumbnails -name "*.png" | wc -l) webp_count=$(find /images/applications/thumbnails -name "*.webp" | wc -l) echo "Number of .png files in thumbnails directory: $png_count" echo "Number of .webp files in thumbnails directory: $webp_count"Length of output: 650
Script:
#!/bin/bash # Search for the FAQ template image in the repository fd "faq-template-wordpress\.png" --type fLength of output: 101
Script:
#!/bin/bash # Check number of .png and .webp files in thumbnails directory thumbnail_dir="images/applications/thumbnails" png_count=$(find "$thumbnail_dir" -type f -name "*.png" | wc -l) webp_count=$(find "$thumbnail_dir" -type f -name "*.webp" | wc -l) echo "Number of .png files in thumbnails directory: $png_count" echo "Number of .webp files in thumbnails directory: $webp_count"Length of output: 428
99-99: LGTM: Updated image path for Certificate templateThe image source path for the Certificate template has been updated to follow the new consistent pattern. Note that this image uses the .webp format, which differs from the .png format used in some other templates. Ensure that this is intentional and that the correct image format is being used.
classes/controllers/FrmAppController.php (1)
Line range hint
225-227: LGTM! Good optimization for floating links.The conditional call to
enqueue_floating_linksusing theshould_show_floating_linksmethod is a good optimization. This ensures that floating links assets are only loaded when necessary, potentially improving page load times on pages where they're not needed.
Crabcyborg
changed the title
1 participant
formidable-formsslug from the check. In many cases, theformidablename is used instead.experimentalRunAllSpecsis now set. This allows you to run all specs when using Chrome.