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

Navbar added #28

Merged
merged 22 commits into from
Apr 15, 2022
Merged

Navbar added #28

merged 22 commits into from
Apr 15, 2022

Conversation

suraj-singh127
Copy link
Contributor

image

Added a Navigation bar to traverse the page.

@yashika51 and @PAOK-2001 please review the changes

@netlify
Copy link

netlify bot commented Apr 14, 2022

👷 Deploy request for prep-22-p3-1-project pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7bea02f

Copy link
Collaborator

@yashika51 yashika51 left a comment

Choose a reason for hiding this comment

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

We should add a .gitignore file ignoring environment files and cache files. I gave the same suggestion here #27 (review)

Adding it to either of the PR would solve the purpose.

@suraj-singh127
Copy link
Contributor Author

suraj-singh127 commented Apr 15, 2022

.eslint file keeps generating everytime the project is locally. This issue is still open facebook/create-react-app#10306 and has not been resolved yet.

A fix is deleting the .eslintcache file before commiting the changes pushing them upstream.

@PAOK-2001
Copy link
Contributor

The nav bar looks great! And I see you have integrate some of the changes I made to avoid conflicts on merge! Great job!

@suraj-singh127
Copy link
Contributor Author

suraj-singh127 commented Apr 15, 2022

After final changes -

screenshot (7)

This PR closes issues #2 #13
Includes changes from #29 pr made by @PAOK-2001 and @Mayank17M

Copy link
Member

@Mayank17M Mayank17M left a comment

Choose a reason for hiding this comment

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

This looks great!
The images can be moved to a different folder. Something like ./assets/images/cloud.jpeg. Also, there should be a separate component for backgroundArray

@suraj-singh127
Copy link
Contributor Author

I moved the images to a backgroundArray component and changed the imports
@Mayank17M @PAOK-2001 @yashika51 can you please review.

Copy link
Contributor

@PAOK-2001 PAOK-2001 left a comment

Choose a reason for hiding this comment

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

Looks ready to merge! Good job. The changes Mayank help give better structure to the proyect

@yashika51 yashika51 merged commit 85cbed4 into MLH-Fellowship:main Apr 15, 2022
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