-
Notifications
You must be signed in to change notification settings - Fork 14
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
docs(812): add accessibility statement to docs site #879
Conversation
You can preview these changes on: |
describe('Desktop view', () => { | ||
it('pages should have footer links', () => { | ||
cy.mockConsentAndVisit('/'); | ||
cy.contains('a', 'Careers').should('exist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good e2e test? I'm not so sure, I agree that these won't change that frequently and it's possibly not much of an issue but what feels off to me is that it's very tightly coupled to the content, conceptually to me, this is very close to a snapshot test 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change pushed to remove footer-navigation.cy.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the only useful test in the header navigation is should contain burger menu when on mobile
. the rest are kind of dom checkings that can and should be in jest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mutebg As requested, new header cypress tests removed as well
f799112
to
86d2b5a
Compare
closes #812
What
I have done:
I have tested manually:
Before:
After:
Who should review this PR:
How to test: