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.

Sign up for GitHub

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

Jump to bottom

feat: convert academic honstey tests to Playwright #54855

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: convert academic honstey tests to Playwright #54855

wants to merge 4 commits into from
+75 −82

Conversation

Sembauke
Copy link
Member

@Sembauke Sembauke commented May 17, 2024 •
edited

Checklist:

Closes #XXXXX

@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. labels May 17, 2024
@Sembauke Sembauke marked this pull request as ready for review May 17, 2024 10:48
@Sembauke Sembauke requested a review from a team as a code owner May 17, 2024 10:48
ojeytonwilliams
ojeytonwilliams requested changes May 17, 2024
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Thanks @Sembauke, this looks good. We can take the opportunity to tidy things up, though:

e2e/academic-honesty.spec.ts Outdated Show resolved Hide resolved
e2e/academic-honesty.spec.ts Outdated Show resolved Hide resolved
e2e/academic-honesty.spec.ts Outdated Show resolved Hide resolved
e2e/academic-honesty.spec.ts Outdated Show resolved Hide resolved
e2e/academic-honesty.spec.ts Outdated Show resolved Hide resolved
Sembauke and others added 2 commits May 17, 2024 13:58
@Sembauke @ojeytonwilliams
Apply suggestions from code review
0d2cb38 
Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
@Sembauke
fix: add recommendations form Oliver
70c9ae2
ojeytonwilliams
ojeytonwilliams requested changes May 17, 2024
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Thanks, Sem. Could you change that one expect, please?

@Sembauke
Copy link
Member Author

Sembauke commented May 17, 2024 •
edited

I can't directly use the translations as the CloseX (the close button) is also part of the text. Thus I used regex to match part of the string instead.

@moT01 moT01 added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label May 20, 2024
@Sembauke Sembauke mentioned this pull request May 21, 2024
Open
61 tasks
@moT01 moT01 added status: merge conflict To be applied to PR's that have a merge conflict and need updating and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels May 31, 2024
moT01
moT01 approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM after the conflict is fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moT01 moT01 moT01 approved these changes

@ojeytonwilliams ojeytonwilliams Awaiting requested review from ojeytonwilliams

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: merge conflict To be applied to PR's that have a merge conflict and need updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Sembauke @ojeytonwilliams @moT01