Add eslint, prettier; refactor code#64
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've made a great start on this task, and the Page Object Model is well-implemented. However, some changes are needed before this can be approved. The main issue is in eslint.config.mjs, where eslint-config-prettier must be the last element in the array to correctly override other style rules. Additionally, a test in tests/signUp/signUpNegative.spec.js will likely fail because the multi-line error message string contains extra whitespace and newlines that won't match the actual text on the page. There are also a few remaining linting errors, such as a line exceeding the maximum length in tests/signUp/signUpPositive.spec.js. Addressing these configuration and test issues will ensure all requirements are met.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| /** @type {import('eslint').Linter.Config[]} */ | ||
| export default [ | ||
| { languageOptions: { globals: globals.node } }, | ||
| eslintConfigPrettier, |
There was a problem hiding this comment.
According to the eslint-config-prettier documentation, this configuration should be placed last in the array. This ensures it can properly override any conflicting style rules from other configurations like @eslint/js and eslint-plugin-playwright.
| ...pluginJs.configs.recommended, | ||
| ...playwright.configs['flat/recommended'], | ||
| rules: { | ||
| ...pluginJs.configs.recommended.rules, |
There was a problem hiding this comment.
The rules from pluginJs.configs.recommended are already included by spreading the configuration object on line 12. Spreading the rules again here is redundant and can be removed.
| comments: 80, | ||
| }, | ||
| ], | ||
| ...playwright.configs['flat/recommended'].rules, |
There was a problem hiding this comment.
Similar to the JavaScript rules, the Playwright-specific rules are already included by the spread on line 13. This makes this line redundant and it should be removed.
| email: faker.internet.email(), | ||
| password: faker.internet.password(), | ||
| } | ||
| username: `${faker.person.firstName()}_${faker.person.lastName()}`, |
There was a problem hiding this comment.
This line exceeds the 80-character limit defined in the eslint.config.mjs file. To fix this, you could create separate variables for the first and last names and then combine them.
| await signUpPage.assertErrorMessageContainsText( | ||
| `username:Username must start with a letter, have no spaces, and be 2 - 40 characters.` | ||
| `username:Username must start with a letter, | ||
| have no spaces, and be 2 - 40 characters.`, | ||
| ); |
There was a problem hiding this comment.
When you create a string that spans multiple lines using a template literal, the newline character and any leading whitespace (indentation) on the following lines are included in the string itself. This will likely cause the toContainText assertion to fail because the error message on the web page won't contain this extra formatting. Consider concatenating the strings or writing the full string on a single line to fix this.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully configured ESLint, Prettier, and Husky, and addressed nearly all the feedback from the previous review. The project is in great shape, and the automated checks are working as expected.
I am approving your solution, but please address one remaining minor linting issue before your next commit:
- In
tests/signIn/signInNegative.spec.js, one line exceeds the 80-character limit. You can resolve this by moving the error message string into a constant, just as you did in the sign-up tests.
This is a great demonstration of setting up and adhering to a project's quality standards. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| await signInPage.fillEmailField('test@gmail.com'); | ||
| await signInPage.clickSignInButton(); | ||
| await signInPage.assertErrorMessageContainsText(`password:can\'t be blank`); | ||
| await signInPage.assertErrorMessageContainsText(`password:can't be blank`); |
There was a problem hiding this comment.
This line exceeds the 80-character limit defined in eslint.config.mjs, especially when accounting for indentation. To resolve this, you could move the error message string into a constant.
2 participants
No description provided.