search

konradkalemba / korona.ws

🗺 Coronavirus interactive map of Poland
https://korona.ws
73 stars 29 forks source link

Refactor DataElement component #15

Closed nowNick closed 4 years ago

nowNick commented 4 years ago

Hi!

First of all: Great job guys! 👏

I was planning on adding some charts to the historical data but first I wanted to understand how things work and I noticed I could add a few optimizations to DataElement component. I extracted common elements and made sure prepareData is called only when cases or deaths changes.

I know it's not a lot but I wanted to start here so that things might be a bit easier in the future.

Please let me know what you think 🙃

PS: Would you consider adding prettier ? I'm not sure if I followed your style guide and I think adding it may help future contributors 🤔

vercel[bot] commented 4 years ago

This pull request is being automatically deployed with ZEIT Now (learn more). To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/konradkalemba/korona-ws/mlv68cz29 ✅ Preview: https://korona-ws-git-fork-nownick-feature-extractcommonparts.konradkalemba.now.sh

konradkalemba commented 4 years ago

Hi, thanks for kind words and your pull request!

I agree, there are components that need refactoring and DataElement is definitely one of them. I will review the changes tonight, but it looks good at the first glance. 👍

Regarding prettier - yes, absolutely, I will try to configure it tonight as well.

konradkalemba commented 4 years ago

I noticed the problem with the latest cases order, but I don't know why it is like that. Check the preview deployment.

nowNick commented 4 years ago

@konradkalemba oh good catch! I just pushed a fix.

I accidentally removed slice and reversed when refactoring.

konradkalemba commented 4 years ago

Looks good! :) Thanks for your contribution.

mhajder commented 4 years ago

These changes probably ruined "Informacje". There is an error after clicking.

konradkalemba commented 4 years ago

@mhajder I don't see any problem. Where did you encounter that problem? This pull request hasn't been deployed to production yet.

mhajder commented 4 years ago

However, it's API's fault. When API rate limit exceeded for GitHub, you cannot open the information page.

konradkalemba commented 4 years ago

@mhajder Oh, I see. I thought contributors endpoint in GitHub's API is public and therefore doesn't have limitations. I'll add an API key and we'll see how it goes. Thanks for the info!

mhajder commented 4 years ago

@konradkalemba I don't know if it's a good idea to add the API key to GH publicly. But you could do a conditional error-catching function here. I will try to do this. https://github.com/konradkalemba/korona.ws/blob/4a991d3cf053d72d879d7e66c35290df1d39e6d3/src/components/Contributors/Contributors.jsx#L10-L13

konradkalemba commented 4 years ago

@mhajder I didn't plan to do so - I wanted to store it as an environment variable. But now thinking about it, my token still would be exposed as it would be inserted into the code during build process. Thanks for the heads-up.

Catching error will definitely solve the problem with popup not showing. 👍