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

Added missing isVisible command in new Element API #4039

Merged
merged 22 commits into from
Mar 8, 2024

Conversation

subhajit20
Copy link
Contributor

Thanks in advance for your contribution. Please follow the below steps in submitting a pull request, as it will help us with reviewing it quicker.

  • Create a new branch from master Add missing isVisible command in new Element API #4037
  • If you're fixing a bug also create an issue if one doesn't exist yet;
  • If it's a new feature explain why do you think it's necessary. Please check with the maintainers beforehand to make sure it is something that we will accept. Usually we only accept new features if we feel that they will benefit the entire community;
  • Please avoid sending PRs which contain drastic or low level changes. If you are certain that the changes are needed, please discuss them beforehand and indicate what the impact will be;
  • If your change is based on existing functionality please consider refactoring first. Pull requests that duplicate code will most likely be ignored;
  • Do not include changes that are not related to the issue at hand;
  • Follow the same coding style with regards to spaces, semicolons, variable naming etc.;
  • Always add unit tests - PRs without tests are most of the times ignored.

@subhajit20 subhajit20 changed the title Added missing isVisible command in new Element API Added missing isVisible command in new Element API Feb 22, 2024
@garg3133
Copy link
Member

Can you please avoid un-related formatting changes in the PR?

@subhajit20
Copy link
Contributor Author

subhajit20 commented Feb 22, 2024

hey @garg3133 , would I close this pr and raise a new pr because I am confused how to reformatt all the unwanted changes?

lib/api/web-element/isVisible.js Outdated Show resolved Hide resolved
test/src/api/commands/web-element/testIsVisisble.js Outdated Show resolved Hide resolved
types/web-element.d.ts Outdated Show resolved Hide resolved
@garg3133
Copy link
Member

Also, please write a different commit message for every commit based on what you've changed in that commit. Good commit messages help navigate a larger PR more effectively.

@subhajit20
Copy link
Contributor Author

Hey @garg3133 I removed all the unwanted commits and removed all the formatted issues.
Please let me know if any further changes are needed.

@garg3133
Copy link
Member

There are still a lot of things wrong here. Please go through the review comments of #4038 and act accordingly.

@garg3133
Copy link
Member

Also, you can run npm test and npm run test:types to run the unit tests locally.

@subhajit20 subhajit20 requested a review from garg3133 February 23, 2024 15:29
@subhajit20
Copy link
Contributor Author

Also, you can run npm test and npm run test:types to run the unit tests locally.

Hey I have made changes in test file and added JSDoc

@garg3133
Copy link
Member

@subhajit20 I have gone through your PRs multiple times and suggested changes, but you don't seem to work very well on the feedback. Like I asked you to go through all the review comments in PR #4038 and act accordingly, but still I see an example in JSDoc which is not BDD-styled (as suggested in the other PR) and moreover, the example would fail when on running it which shows that you haven't even taken the effort of running the example you're putting in the doc by yourself first. Also, the test is not in the right format (on which I commented at the end of PR $4038) but you seem to have just copy-pasted the test from the other PR without paying attention to the comments.

So, I'm not going to put any more reviews on your PRs until you work on the existing reviews first. Plus, I'm going to open up the issue for isVisible command to other contributors and let you work on the reviews for isEnabled command first before assigning any more issues to you.

We're looking for people who can solve stuff and also test their changes thoroughly in their local environments before asking someone to come and review their PR, and also work on the feedback given.

You are of course welcome to ask questions on Discord if you don't understand something.

@subhajit20
Copy link
Contributor Author

Hey @garg3133 can you point out those sections which are needed to be resolved?
It would be very helpful then

@subhajit20
Copy link
Contributor Author

hey sorry @garg3133 , actually I was bit confused.Can you check once I have made some changes and tried to run test.
It was successfully executed.
Screenshot 2024-02-24 at 3 14 32 PM

@garg3133
Copy link
Member

Hey @garg3133 can you point out those sections which are needed to be resolved?

I already did that in the comment posted above -- JSDoc and tests.

@subhajit20
Copy link
Contributor Author

Hey @garg3133 can you point out those sections which are needed to be resolved?

I already did that in the comment posted above -- JSDoc and tests.

Yeah I saw those and made some changes according to this #4038 pr.

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

I would recommend adding few negative test cases as well.

test/src/api/commands/web-element/testIsVisible.js Outdated Show resolved Hide resolved
@dikwickley dikwickley mentioned this pull request Feb 25, 2024
8 tasks
@subhajit20
Copy link
Contributor Author

Hey @garg3133 can you please now review my pr? I have done all the required changes

lib/api/web-element/commands/isVisible.js Outdated Show resolved Hide resolved
lib/api/web-element/commands/isVisible.js Outdated Show resolved Hide resolved
test/src/api/commands/web-element/testIsVisible.js Outdated Show resolved Hide resolved
test/src/api/commands/web-element/testIsVisible.js Outdated Show resolved Hide resolved
@subhajit20
Copy link
Contributor Author

Hey @vaibhavsingh97 I got all those above changes done!!

@vaibhavsingh97
Copy link
Member

@subhajit20 Can you please resolve conflicts in the PR

Copy link

github-actions bot commented Mar 1, 2024

Status

  • ✅ Type files updated!

@subhajit20
Copy link
Contributor Author

@subhajit20 Can you please resolve conflicts in the PR

Hey I have resolved the conflit issue.
Please let me know if any other chnages needed.

Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

The tests are completely wrong. Also, the alias for isDisplayed() command is missing.

lib/api/web-element/commands/isVisible.js Outdated Show resolved Hide resolved
test/src/api/commands/web-element/testIsVisible.js Outdated Show resolved Hide resolved
test/src/api/commands/web-element/testIsVisible.js Outdated Show resolved Hide resolved
@garg3133
Copy link
Member

garg3133 commented Mar 1, 2024

It looks like you recently changed the tests, may I know the reason for doing that?

@subhajit20
Copy link
Contributor Author

It looks like you recently changed the tests, may I know the reason for doing that?

Yes I am reforming the test file as it got broken while I was resolving the merge conflict issue . I got stuck and just reimplentiting the test and command file. Will do that by tomorrow

@subhajit20
Copy link
Contributor Author

hey @garg3133 can you review this pr?

@garg3133 garg3133 merged commit 9514af4 into nightwatchjs:main Mar 8, 2024
17 checks passed
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.

3 participants