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

Search data attribute in parent elements #4

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmoliveira
Copy link

@fmoliveira fmoliveira commented Oct 2, 2019

Pull Request Template

Fixes issue #1 .

Description

I will add more details soon.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I've used breakpoints while running the unpacked extension, as well as successfully recorded a lot of tests with the proper data attributes.

I'll try to add a unit test to cover this and will update this PR soon.

Checklist:

  • My code follows the style guidelines of this project. npm run lint passes with no errors.
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes. npm run test passes with no errors.

@fmoliveira
Copy link
Author

I can't run linting. Blocked by issue #5

@fmoliveira fmoliveira changed the title Search data attribute in parent elements. Search data attribute in parent elements Oct 3, 2019
@wolfi3b
Copy link

wolfi3b commented Dec 3, 2019

@fmoliveira Hi, I am very much looking for your PR to be merged and available in the extension. Is there any way I could help?

@fmoliveira
Copy link
Author

@wolfi3b Sure thing! Thanks for coming in!

This PR is currently blocked because I can't tick all the checklist in the Pull Request Template. It has a lot of linting errors, but all those errors also happen on master! I'm wondering if I just have issues with my setup, or if we actually have those lint errors to solve. I've opened an issue at #5

Could you please checkout the repository in the master branch, install the dependencies with npm installand runnpm run lint` to confirm the issue #5 ?

If that issue is confirmed, then we can work on getting the linting issues fixed, and unblock the PR checklist. :)

@wolfi3b
Copy link

wolfi3b commented Dec 4, 2019

No problem. So I run it and got ton of lint errors as well so I would say something is not right here. Maybe author forgot to commit something.

@wolfi3b
Copy link

wolfi3b commented Dec 4, 2019

Well if I run it with --fix than only 40 remains :-)

@fmoliveira
Copy link
Author

@wolfi3b Awesome!! I haven't tried the auto fix, very good to hear that!!

Would you open a PR with these fixes? I can help you fixing the remaining ones if you'd like. :)

@fmoliveira
Copy link
Author

@oscartavarez Could you please confirm if the linting settings are properly committed to the repository? Do you have the same errors on your environment as well, or may the repository be missing a file with your linting settings?

@wolfi3b
Copy link

wolfi3b commented Dec 4, 2019

@fmoliveira Well the thing is ..I don't know if I should. I mean for all I know the issues are here because the author forgot to commit linting settings. It would be strange he did not run lint on his code.
If we run it as
npm run lint -- --fix
it will fix all but those 40. Which most are simple to fix from what I checked. So if we get green from @oscartavarez to go for that it should be easy and I have no problem to create PR for all of those issues together.

But correct me if I am wrong he does need to confirm the PR am I right? So we still need him to check our comments.

@fmoliveira
Copy link
Author

fmoliveira commented Dec 4, 2019

@wolfi3b Yeah, you're right! I thought about the settings afterwards!

I've pinged @oscartavarez in the linting issue at #5 as well, which is shorter, let's wait for an answer - as you've said, he will have to confirm the PR anyway before we can merge this fix. 🙃

@wolfi3b
Copy link

wolfi3b commented Dec 6, 2019

@fmoliveira hmmm...do you know if it is possible to check somewhere if anyone else than @oscartavarez has write access to confirm PR's? I mean...his last activity is more than 2 months ago (if I did not miss anything) suggesting he is pretty busy and may not have the time or opportunity to check here anytime soon.

@fmoliveira
Copy link
Author

@wolfi3b We can see the list of contributors, but I think it's not possible to see list of maintainers.

@tnolet Do you think you could help us? We would like to confirm whether the repository is missing the proper lint settings (because it has hundreds of lint errors), and afterwards, we would want to get a couple of pull requests reviewed and merged (they are both blocked by the lint issue).

@tnolet
Copy link
Contributor

tnolet commented Dec 9, 2019

@fmoliveira was not aware of this "fork" of Puppeteer Recorder. Cool!
I had 5 lint errors in Puppeteer Recorder. Also, I was not checking for this in the Travis CI build. I just pushed a small update. I have no time to comb through the differences between this and my repo.

@fmoliveira
Copy link
Author

@tnolet Sweet, I wasn't aware of the Puppeteer Recorder either, I will definitely have a look on it! Since this repo is not tagged as a fork (probably it was cloned and pushed instead), I imagined you were an an early member of this project.

Thanks for your answer! 😃

@wolfi3b
Copy link

wolfi3b commented Dec 11, 2019

That is strange ...I had a quick look and the config files looks pretty the same with the pupeteer repo. So is it possible the author of this "fork" just did not run lint?

@fmoliveira
Copy link
Author

@wolfi3b Yes, I don't doubt that! The CONTRIBUTING.md and PULL_REQUEST_TEMPLATE.md only had the word Pupeeteer replaced for Cypress and it's all the same.

@tnolet
Copy link
Contributor

tnolet commented Dec 11, 2019

Hey guys, just check the Puppeteer Recorder Travis build.

https://travis-ci.org/checkly/puppeteer-recorder

It's green and runs npm run lint. Maybe there some hint there.

@fmoliveira
Copy link
Author

@tnolet Nice, I'll take a look! Thanks!! 🙌

@wolfi3b
Copy link

wolfi3b commented Jan 8, 2020

@fmoliveira hi, I just got back...any luck/update?

@fmoliveira
Copy link
Author

Hi @wolfi3b !

No news! But given this repository is a fork from puppeteer-recorder and the former has the same PR template, but there are lots of lint errors here, I believe linting wasn't really taken into account on this repository. We could either fix everything or just forget it and merge anyway.

In any case, we need your help @oscartavarez ! Could you please take a look here or at least add some of us as maintainers and publishers so we could keep improving this tool? Thanks a lot, mate!

@oscartavarez
Copy link
Owner

Sorry guys for the delay, this weekend i will take a look at it. really sorry guys @fmoliveira @wolfi3b.

@fmoliveira
Copy link
Author

@oscartavarez No problem man! Happy to see you around! 😃

@wolfi3b
Copy link

wolfi3b commented Jan 28, 2020

@oscartavarez Glad you are back! If you don't have time I agree with @fmoliveira. It would be great if you could make some of us maintainers so we can help you not having to handle everything yourself.

@wolfi3b
Copy link

wolfi3b commented Feb 11, 2020

@oscartavarez Hi did you have any time to check it out? Or if you could make one of us able to approve we would very much appreciate it.

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.

4 participants