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

Verifying AI popup for six month old unlaunched sites - PS0-2221 #31

Merged
merged 12 commits into from
Oct 31, 2024

Conversation

asr0393
Copy link
Contributor

@asr0393 asr0393 commented Oct 17, 2024

Proposed changes

https://jira.newfold.com/browse/PRESS0-2221 - Add Cypress Test for Modal Notification

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Video

Checklist

  • I have read the CONTRIBUTING doc
  • I have viewed my change in a web-browser
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@asr0393 asr0393 requested review from circlecube and wpalani October 17, 2024 12:56
@asr0393 asr0393 requested a review from arunshenoy99 October 25, 2024 12:29
it( 'Is Accessible', () => {
cy.injectAxe();
cy.wait( 1000 );
cy.checkA11y( '.newfold-notifications-wrapper' );
cy.checkA11y( '.ai-sitegen-modal__header' );
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this target was updated. I think it should remain as it was. It was looking at the notifications wrapper to ensure it is a11y compliant. I'm not sure why it would need to change, and I think targeting the ai sitegen modal header is not correct. I see the new notification is added, but it would load inside the existing wrapper, web plugin caught this since I don't believe it uses ai sitegen. Can we change this target back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted that to '.newfold-notifications-wrapper', Also have added alt text to images which were missing it.

@asr0393 asr0393 requested a review from circlecube October 30, 2024 09:44
@circlecube
Copy link
Member

This still looks to be failing the accessibility check on Web.

… bluehost

update notice id to reflect content rather than number for clarity
lint auto-fixes in test file
consistently use cypress env value for pluginId, no external support file necessary
this was changed in the previous commit because the onboarding screen is no longer in the app and it was updated to remove the page and content to reflect that.
@circlecube
Copy link
Member

I updated the tests and see that they are all passing.

I noticed the web test was failing because the ai modal was displaying, which it should only display for bluehost. I updated the fixture to only add the ai notice on bluehost and skip those tests entirely unless bluehost too.

I also cleaned the tests up a bit and updated the notice ids to be more descriptive and fixed the auto-lint issues.

@circlecube circlecube merged commit 75fa857 into main Oct 31, 2024
7 checks passed
@circlecube circlecube deleted the ps0-2221 branch October 31, 2024 18:09
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.

2 participants