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

Superhero-Search-Project #3

Merged
merged 9 commits into from
Oct 14, 2023
Merged

Superhero-Search-Project #3

merged 9 commits into from
Oct 14, 2023

Conversation

bhushan354
Copy link
Owner

In This PR I have used Following things while implementing features in my webApp :

  • Correct GitFlow Used.
  • Used React documentation.
  • Used React components.
  • Used React props.
  • Used React Router.
  • Connected React and Redux.
  • Handled events in a React app.
  • Wrote integration tests with a React testing library.
  • Used styles in a React app.
  • Used React life cycle methods.
  • Applied React best practices and language style guides in code.
  • Used store, actions and reducers in React.

Thank you ,
Bhuhsan .

Copy link

@skyv26 skyv26 left a comment

Choose a reason for hiding this comment

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

Hi @bhushan354,

Good job so far 👍🏻
There are some issues that you still need to work on to go to the next project but you are almost there!

STATUS: REQUIRE CHANGES 🔴

Highlights

  • Good PR title ✅
  • Descriptive PR description ✅
  • Implement the features ✅

Required Changes ♻️

  • Because your file changes are not reflected in this PR, that's why I am gonna add some comments here. I appreciate your hard work, you did an amazing job by adding your creativity, and design logic and I am happy that instead of 2 you made three pages. But you should stick with project requirements, by strongly following the design wireframe, according to which, you should have 2 column design only, and you need to make sure that your application should use only one colour theme, not multiple colours so that's why you need to make some updates to meet the project requirement, your second page I agreed you have two columns but your first page should also have the same design as second. Kudos for the search functionality which is working fine.

image

  • As I mentioned about colour, you can choose the colour of your own choice but your layout should also use the same colour except for the colours for the text and links. I can see that you
    are using more than 2 colours, so please adhere to the requirements and follow the wireframe colour properly.

image

  • Kindly take a look and compare your design, your design looks different in terms of styling, please fix your design styles because adhering to the project requirement will help you to be a better developer and detail-oriented

Design Comparison

Behance Design Your Design
metrics_app

image

  • Please make sure that your application is warm or error-free, sometime Warnings are not that important for us while development, but if you are getting a warning for something really important then you should fix it because as you can see in the below screenshot, you got a warning of missing attribute Key for the element running inside the loop but because React uses the virtual dom and always make the required changes to the real DOM and behind the scenes, this type of keys help the react to differentiate between the things and maintain the uniqueness to perform the render or repaint the component based on our action.

image

  • As I mentioned already about your design, I would also take your attention to your detail page, you should make your detail page as per the wireframe provided by Microverse, it seems like you put in your efforts but neglected the important requirement, but I still believe you can do make the changes to pass the requirements.

image

  • Always try to separate your test files from your components, you test file should be inside the __test__ folder just like component folders which makes your code base clean and easy to follow.

image

Check the comments under the review.

Optional suggestions

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you take them into account as they can make your code better.

Cheers, and Happy coding!👏👏👏

⚠️ WARNING ⚠️

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.

Have any doubt ❓

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag @skyv26 in your question so I can receive the notification. You can also follow me on the below platforms

@skyv26 linkedin-skyv2022 @vrma_aakash @skybrel @skybrel

As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.

README.md Outdated
Copy link

@skyv26 skyv26 Oct 14, 2023

Choose a reason for hiding this comment

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

  • Please add the video presentation link as per the requirement, your video should not be more than 5 min. You can use the loom for recording and then make a public shareable link. So that we as CR can look into that in order to get a review of what you did and what is special about your project. Kindly stick with the requirements tightly.

Choose a reason for hiding this comment

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

  • Well resolved

README.md Outdated
Copy link

@skyv26 skyv26 Oct 14, 2023

Choose a reason for hiding this comment

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

  • Kindly always make sure that your README file is professional and has everything in a well-structured way as the things provided by Microverse in a well-documented way. Your project description seems irrelevant to the project and needs the proper formatting with a short description of your project.

image

Choose a reason for hiding this comment

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

  • [REQUIRED] Kindly add a professional project description to this project. The previous reviewer only recommended that you add the relevant description to the README not that you should remove it completely. A professional README needs a standard project description.


## 🚀 Live Demo <a name="live-demo"></a>

- [[Live Demo link](https://bhushan354.github.io/Superhero-Search-Project/)]
Copy link

Choose a reason for hiding this comment

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

  • I noticed that your live link is not working, rather it shows your actual live web app, it just shows your README file, which is totally unprofessional, irrelevant and not acceptable as per the requirement. Kindly deploy your project properly and share the correct deployed link in the README file.

image

Copy link

@Whoistolu Whoistolu Oct 14, 2023

Choose a reason for hiding this comment

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

  • [REQUIRED] This project is still yet to be deployed as recommended by the previous reviewer and as stated by the project requirement. Kindly deploy your project properly and share the correct deployed link in the README file. Kindly deploy your project properly and add the deployment link here.

Comment on lines -96 to -100

Contributions, issues, and feature requests are welcome!

Feel free to check the [issues page](https://github.com/bhushan354//issues)

Copy link

@skyv26 skyv26 Oct 14, 2023

Choose a reason for hiding this comment

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

  • Please make sure that you don't have any broken links in your README file because I tried to see your contributing link which is supposed to take me to your repo issue section, but I got a 404 page which means the path that you provided in your README file is not correct, kindly fix it.

image

Choose a reason for hiding this comment

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

  • well resolved

Comment on lines -112 to -114
## 🙏 Acknowledgments <a name="acknowledgements"></a>

I would like to thank the Microverse team and Code reviewers who helped us create this project. Also, I would like to express our sincere gratitude to [Agustine Soria](https://github.com/SaveryIV) for his valuable assistance in solving a problem with fetching rockets From API. His expertise and guidance Ire instrumental in finding a solution.
Copy link

@skyv26 skyv26 Oct 14, 2023

Choose a reason for hiding this comment

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

  • Well done, I really appreciate your sincerity in acknowledging your peer/fellow/friend who helped you with the project but as per the requirement, you should acknowledge first the designer of the web application from whom you took the inspiration to design the application. Please follow the requirements carefully and update this section with acknowledgement for the designer of the metrics apps.

image

Choose a reason for hiding this comment

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

  • well resolved

@bhushan354 bhushan354 temporarily deployed to github-pages October 14, 2023 19:16 — with GitHub Pages Inactive
@bhushan354 bhushan354 temporarily deployed to github-pages October 14, 2023 19:48 — with GitHub Pages Inactive
@bhushan354 bhushan354 temporarily deployed to github-pages October 14, 2023 19:54 — with GitHub Pages Inactive
@bhushan354
Copy link
Owner Author

Hello sir , I hope these changes fullfills the criteria but if there are some issues , it would be of great help to me if you tell me on whatsapp +918600118932 , as I have time constraint .

Thank you for consideration,
Bhushan .

Copy link

@Whoistolu Whoistolu left a comment

Choose a reason for hiding this comment

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

Hi @bhushan354

Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!

Required Changes ♻️

  • Check the inline comments under the review for sections to improve.

Optional suggestions

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

N/A

Cheers and Happy coding!

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me @Whoistolu in your question so I can receive the notification.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.


As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.



## 📝 License <a name="license"></a>

This project is [MIT](./MIT) licensed.
This project is [MIT](./LICENSE) licensed.

Choose a reason for hiding this comment

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

  • Well resolved

@@ -96,7 +94,7 @@ To get a local copy up and running follow these simple example steps.

Contributions, issues, and feature requests are welcome!

Feel free to check the [issues page](https://github.com/bhushan354//issues)
Feel free to check the [issues page](https://github.com/bhushan354/Superhero-Search-Project/issues)

Choose a reason for hiding this comment

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

  • Well resolved

@bhushan354
Copy link
Owner Author

hello sir you have written well resolved in here so what does it mean @Whoistolu

@Whoistolu
Copy link

Whoistolu commented Oct 14, 2023

Hello sir , I hope these changes fullfills the criteria but if there are some issues , it would be of great help to me if you tell me on whatsapp +918600118932 , as I have time constraint .

Thank you for consideration, Bhushan .

You are almost there @bhushan354 🥇 . Kindly check the comments I added for the remaining changes under the comments of the previous reviewer. The project description on the README and the deployment are still pending. Let's fix those 👍🏽

@bhushan354
Copy link
Owner Author

bhushan354 commented Oct 14, 2023

sir I dont know why the github pages is deploying the README @Whoistolu , can you help me with that , please!!

@Whoistolu
Copy link

Whoistolu commented Oct 14, 2023

sir I dont know why the github pages is deploying the README @Whoistolu , can you help me with that , please!!

So sorry about this experience @bhushan354 , kindly ensure that you have selected the correct branch to deploy from. Pease ensure that you are selecting the feature branch.

@bhushan354
Copy link
Owner Author

bhushan354 commented Oct 14, 2023

yes sir I have selected it correctly @Whoistolu

@Reem-lab
Copy link

Hello @bhushan354 could you check your slack?

@bhushan354
Copy link
Owner Author

bhushan354 commented Oct 14, 2023

Hello sir/mam , I hope these changes fullfills the criteria but if there are some issues , it would be of great help to me if you tell me on whatsapp +918600118932 , as I have time constraint and have to submit this project in less than 1 hour and my everything else is completed .

Thank you for consideration,
Bhushan .

@bhushan354
Copy link
Owner Author

Hello @bhushan354 could you check your slack?

yes wait

@bhushan354 bhushan354 merged commit 72c005b into Developement Oct 14, 2023
Copy link

@Omar-Muhamad Omar-Muhamad left a comment

Choose a reason for hiding this comment

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

Changes Required ♻️

Hi @,

Nice job, you are almost there.

cat-driving-serious

(Highlights) Good Points: 👍

  • Your work is well documented. ✔
  • The correct workflow is used. ✔
  • All linters checks are ok. ✔

(Changes Required) Aspects to improve: ♻️

  • Please check the required changes here:
    As required by the last reviewer, Please add a proper project description here to the readme file.

image

[Optional] suggestions:

  • Nothing to mention.

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

Cheers and Happy coding!👏👏👏

Feel free to leave any questions or comments in the PR thread if something is not 100% clear, and please remember to tag me in your question so I can receive the notification**

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.


As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.

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