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

Feature: App running with npm install #1291

Merged
merged 14 commits into from
Jan 10, 2024
Merged

Feature: App running with npm install #1291

merged 14 commits into from
Jan 10, 2024

Conversation

git-init-priyanshu
Copy link
Member

@git-init-priyanshu git-init-priyanshu commented Dec 26, 2023

What kind of change does this PR introduce?
feature

Issue Number:
Fixes #1246

Did you add tests for your changes?
No

Snapshots/Videos:
Before:
image
After:
image

If relevant, did you update the documentation?
Yes

Summary
Right now, we cannot use npm install as it throws a bunch of errors, and can only be solved by using the --legacy-peer-deps flag. This PR fixes those peer dependency issues.

Does this PR introduce a breaking change?
No.

Have you read the contributing guide?
Yes

Copy link

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

  1. The overall code coverage drops below the target threshold of the repository
  2. Any file in the pull request has code coverage levels below the repository threshold
  3. Merge conflicts

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

  1. PRs with issues not assigned to you will be closed by the reviewer
  2. Fix the first comment in the PR so that each issue listed automatically closes

Other

  1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
  2. Read the CONTRIBUTING.md file make

@git-init-priyanshu git-init-priyanshu changed the title Removed package and updated NSTALLATION.md Feature: App running with npm install Dec 26, 2023
Copy link

codecov bot commented Dec 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3819bd8) 96.99% compared to head (9e95ecd) 97.44%.
Report is 12 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1291      +/-   ##
===========================================
+ Coverage    96.99%   97.44%   +0.45%     
===========================================
  Files          137      138       +1     
  Lines         3428     3683     +255     
  Branches       964     1076     +112     
===========================================
+ Hits          3325     3589     +264     
+ Misses          98       89       -9     
  Partials         5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aashimawadhwa
Copy link
Member

can you share a video of how npm i is working fine now? @git-init-priyanshu

@git-init-priyanshu
Copy link
Member Author

@palisadoes
Copy link
Contributor

Was the enzyme-adapter-react-16 package ever used in the code base? You've only edited the package listing.

@git-init-priyanshu
Copy link
Member Author

Was the enzyme-adapter-react-16 package ever used in the code base? You've only edited the package listing.

No sir, it was not used in the code. The solution was to install @wojtekmaj/enzyme-adapter-react-17 package which is compatible with react 17. It was already installed and correctly used.

@git-init-priyanshu
Copy link
Member Author

Right now I am trying to solve peer depedency warnings. And I also need to change the INSTALLATION.md for the node version.

@git-init-priyanshu
Copy link
Member Author

Hey @aashimawadhwa can you check this PR now?

@palisadoes
Copy link
Contributor

@noman2002 @anwersayeed @aashimawadhwa please review

value="searchText"
// value="searchText"
Copy link
Member

Choose a reason for hiding this comment

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

Why this is commented ?

@git-init-priyanshu
Copy link
Member Author

hey @noman2002 @rishav-jha-mech, Sorry for my late reply, I didn't receive the notification.
I commented those two lines because I was not sure what to do as they were throwing error "Property 'value' does not exist ...". I also checked all the reference of "Dropdown.Item" in the whole codebase and the value property was only defined here. And I also checked the API of "Dropdown.Item" and there was also no mention of value property. Let me know if this is a mistake I will correct it.

@git-init-priyanshu
Copy link
Member Author

hey @rishav-jha-mech @noman2002, I have fixed the issue and updated the INSTALLATION.md file.

@rishav-jha-mech
Copy link
Contributor

hey @noman2002 @rishav-jha-mech, Sorry for my late reply, I didn't receive the notification. I commented those two lines because I was not sure what to do as they were throwing error "Property 'value' does not exist ...". I also checked all the reference of "Dropdown.Item" in the whole codebase and the value property was only defined here. And I also checked the API of "Dropdown.Item" and there was also no mention of value property. Let me know if this is a mistake I will correct it.

It maybe a eslint error, make sure you have eslint extension installed in your VS Code

@rishav-jha-mech
Copy link
Contributor

@git-init-priyanshu after all these changes are the packages getting installed without --legacy-peer-deps with npm instal ?

@git-init-priyanshu
Copy link
Member Author

@git-init-priyanshu after all these changes are the packages getting installed without --legacy-peer-deps with npm instal ?

yes. Here is the video proof. https://drive.google.com/drive/folders/1j66OogD9dgTzPOLHCLYjN_KAP_CCj4sO

@rishav-jha-mech
Copy link
Contributor

rishav-jha-mech commented Jan 5, 2024

@git-init-priyanshu I have seen the uploaded video, let me know if having node-sass module is causing any issue, if not then we will merge your PR

@git-init-priyanshu
Copy link
Member Author

@git-init-priyanshu I have seen the uploaded video, let me know if having node-sass module is causing any issue, if not then we will merge your PR

Right now everything is working fine. But I found some sass files under src/assets/scss , So if these files need to be compiled then I think we do need the node-sass.

@rishav-jha-mech
Copy link
Contributor

@git-init-priyanshu please put node-sass back into package.json, and let us know if the npm install is working or not

@git-init-priyanshu
Copy link
Member Author

@git-init-priyanshu please put node-sass back into package.json, and let us know if the npm install is working or not

I reverted the changes. And npm install is working fine.

@palisadoes palisadoes merged commit 58389f6 into PalisadoesFoundation:develop Jan 10, 2024
9 of 10 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.

5 participants