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

fix: Overlay didn't display "npmjs" pages that do not contain a repository element #149

Merged
merged 10 commits into from
Aug 10, 2023

Conversation

aviv1620
Copy link
Contributor

@aviv1620 aviv1620 commented Aug 7, 2023

fix issue 148.
#148

content.npmjs.js - mount Overlay based on h3 element title "install" instead of repository element.
utils.js - if the element is not in the page. reject timeout after 10 seconds.
make bugs like this more easy to detect next time.
.gitignore - not pull cache and autogenerated files

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Thank you!!!

You did good work, but I want some changes. Please comment if you have any questions.

.gitignore Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/content/content.npmjs.js Outdated Show resolved Hide resolved
src/content/content.npmjs.js Outdated Show resolved Hide resolved
src/utils/utils.js Outdated Show resolved Hide resolved
src/utils/utils.js Outdated Show resolved Hide resolved
aviv1620 and others added 2 commits August 8, 2023 16:26
Co-authored-by: Baruch Odem (Rothkoff) <baruchiro@gmail.com>
@aviv1620
Copy link
Contributor Author

aviv1620 commented Aug 8, 2023

I resolve conversations in the lest commit.

content.npmjs.js - use more safe another selector.
utils.js,cache.js and globals.js - define time in globals.js.
utils.js - solve without wrapping promise in race.
yarn.lock - revert.

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

If you're trying to support yarn v3, please explain

.yarnrc.yml Outdated Show resolved Hide resolved
src/globals.js Outdated Show resolved Hide resolved
src/globals.js Outdated Show resolved Hide resolved
src/utils/utils.js Outdated Show resolved Hide resolved
@aviv1620
Copy link
Contributor Author

aviv1620 commented Aug 8, 2023

Which version of yarn do I need to use?
Until now I run the project on my computer in version 3.

@baruchiro
Copy link
Collaborator

We're using version 1

@aviv1620
Copy link
Contributor Author

aviv1620 commented Aug 9, 2023

OK.
I downgrade to yarn v1.

I run yarn set version 1.22.19. I recommend documenting it in README.md in the second "Local Development" steps.

I revert the yarn.lock file. Now the yarn install does not change it.

I will not ignore it now from the .yarn folder. This should fix the error in the Github tests.
In my computer all the tests work: manually, Jest and end-to-end.

package.json Show resolved Hide resolved
@baruchiro baruchiro changed the title fix: Overlay didn't display "npmjs" pages that do not contain a repository element. fix: Overlay didn't display "npmjs" pages that do not contain a repository element Aug 9, 2023
Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Very good work!!!

FinallySpongeBobGIF

@baruchiro baruchiro added this pull request to the merge queue Aug 10, 2023
Merged via the queue into os-scar:master with commit a274b0b Aug 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants