search

KawemeKowa / weather-progressive-web-app

Simple Progressive Web App. Weather WPA React Application that uses Open Weather API
0 stars 0 forks source link

simple and straight forward #1

Open OlivierJM opened 3 years ago

OlivierJM commented 3 years ago

Hi @KawemeKowa this looks great, it is simple and straight forward and easy to read. here are some improvements you can make:

If you separate your components, it will be easier for you to write tests.

KawemeKowa commented 3 years ago

Thanks for the feedback Oli, let me work on separating the components. I have never written tests so this is a good opportunity to learn.

On Sat, 7 Aug 2021 at 6:45 PM, Oli @.***> wrote:

Hi @KawemeKowa https://github.com/KawemeKowa this looks great, it is simple and straight forward and easy to read. here are some improvements you can make:

  • divide components into separate files
    • SearchInput
    • Weather UI
  • Add tests

If you separate your components, it will be easier for you to write tests.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KawemeKowa/weather-progressive-web-app/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQK4HW457XMTH6RJJ5YSYL3T3VPKZANCNFSM5BXSEWRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

OlivierJM commented 3 years ago

That is true, this will be a good opportunity.

I recorded a video sometime back to share with someone how to do basic testing, you can find it here https://youtu.be/YGLnaltNj1s and the documentation can be found here as well https://testing-library.com/docs/react-testing-library/intro/

KawemeKowa commented 3 years ago

Thanks for the pointers Oli, I’ll learn how to write tests and update the code, during the course of the week

On Sun, 8 Aug 2021 at 10:24 AM, Oli @.***> wrote:

That is true, this will be a good opportunity.

I recorded a video sometime back to share with someone how to do basic testing, you can find it here https://youtu.be/YGLnaltNj1s and the documentation can be found here as well https://testing-library.com/docs/react-testing-library/intro/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KawemeKowa/weather-progressive-web-app/issues/1#issuecomment-894762198, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQK4HWY5GC42ESJZLCV3UYDT3Y5LLANCNFSM5BXSEWRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

OlivierJM commented 3 years ago

👍

KawemeKowa commented 3 years ago

Hey Oli, So I made some changes as per your feedback. I separated the components into separate files and added some simple tests. Thanks for that, it took me back to the basics of prop drilling and introduced to to testing. I will add more tests as I get better at it

OlivierJM commented 3 years ago

Hi Kaweme,

That sounds great, Thanks a lot for making the changes, I will take some time to review it this evening and I will give you feedback.

KawemeKowa commented 3 years ago

Alright, thanks

OlivierJM commented 3 years ago

Hi Kaweme, I took a look at it and it is some good improvement 🚀🚀, here is more you can do:

KawemeKowa commented 3 years ago

Thanks for the great feedback Oli, I will let you know once I make these changes.

On Wed, Aug 18, 2021 at 12:31 AM Oli @.***> wrote:

Hi Kaweme, I took a look at it and it is some good improvement 🚀🚀, here is more you can do:

  • Let the component own its own state
    • SearchInput should manage both query and setQuery since these are not needed outside itself
  • Handle errors
    • Let's say something went wrong while fetching the weather
    • User should see an error if it occurs
    • What happens if the weather is null - currently it would break the whole app.
  • Use PropTypes to protect again errors and guard your props with their respective types, you can find more info here https://reactjs.org/docs/typechecking-with-proptypes.html
  • Add more tests

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KawemeKowa/weather-progressive-web-app/issues/1#issuecomment-900674215, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQK4HW2BME53FH6UVAZFYQDT5LPLXANCNFSM5BXSEWRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

KawemeKowa commented 3 years ago

Hey Oli, I made a few changes as per your suggestions but I have a few areas of concern. I am able to catch the error but I am not sure how to return it back to the calling function to be used so I am just console.logging it. I am however using an error state to check for an error before rendering. I added one propType and I am yet to add more tests. Please go through the changes

OlivierJM commented 3 years ago

Hey Oli, I made a few changes as per your suggestions but I have a few areas of concern. I am able to catch the error but I am not sure how to return it back to the calling function to be used so I am just console.logging it. I am however using an error state to check for an error before rendering. I added one propType and I am yet to add more tests. Please go through the changes

Thanks @KawemeKowa I will go through the changes and give you feedback.

KawemeKowa commented 3 years ago

awesome

On Mon, Nov 15, 2021 at 8:05 AM Oli @.***> wrote:

Hey Oli, I made a few changes as per your suggestions but I have a few areas of concern. I am able to catch the error but I am not sure how to return it back to the calling function to be used so I am just console.logging it. I am however using an error state to check for an error before rendering. I added one propType and I am yet to add more tests. Please go through the changes

Thanks @KawemeKowa https://github.com/KawemeKowa I will go through the changes and give you feedback.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KawemeKowa/weather-progressive-web-app/issues/1#issuecomment-968566826, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQK4HWY2456TBOFM22SS7CLUMCPL7ANCNFSM5BXSEWRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.