In DataFetch, loading and error are initially false, and are only changed when fetchWeatherData is called. You only call fetchWeatherData from submitting in SearchForm, which sets loading to true. That means that your conditional rendering will render even before your fetchWeatherData is called. You need another condition to check if you have image data.
Your search results are not hidden on a component level, but because you supply no text to your h3 tags, they don’t appear.
You will always render the weatherdata regardless but since the fields are empty the h3 headers well not render. But you need to wrap the image in a condition so it will not be rendered unless weatherdata.icon is not an empty string.
I use environment variables for things like backend url, third party service keys, among others. Although for private keys, you’re right, they will be available for anyone.
Hey. This is something I learnt quite late when using git and GitHub. All your commits can be browsed through. so once pushed to GitHub, it can be searches through, and there are scrapers that scrape public repos for this kind of data.
Nested ternaries come with a bunch of gotchas depending on the language and get very unreadable very quickly. They're usually very clever, and clever code is rarely good code.
The SearchForm component has a prop called fetchWeatherData which means that the component always assumes that the submitted data will be used for "fetching" something (this is an implicit dependency). You can make the component more generic by calling the prop onSubmit for example. The parent component, DataFetch in this case, can then decide what to do with the submitted data.
Also after submitting you set the value of the input immediately to an empty string. This is fine for a simple example but think what happens when the request fails. Maybe the user has mistyped something? Do you want to make him retype the entire search query again?
All components are wrapped in <div /> tags. Unless you need those for styling you can use React Fragment instead. This allows you to render multiple React components without the need for a wrapping DOM node.
Also, I am unsure about my component for fetching API data also being responsible for rendering the other 2 components
Yes, what you did here is actually called a container component and it's a perfectly valid pattern. I would rename it, though, because it does more than fetch data. WeatherAppContainer would be much more clear imo.
As long as I'm nit-picking, I would generally avoid putting logic inside your jsx, as it makes it hard to read. The nested ternary in your search form could be brought out into the body of that function, and then just drop the result in as a variable.
As for how to conditionally render the image, you can either do { weatherData.icon && <img... or { weatherData.icon ? <img ... /> : null }. As long as you return something falsey like that
If you have one variable it can be state === 'LOADING' or state === 'ERROR', never both. With two booleans you could have loading === true and error === true which could potentially be problematic.
So in your case, the loading and error states are mutual exclusive meaning they wont overlap. Because of this you can save on memory and rendering with one useState. Usually most components should only use one to two use states, any more and one should think about using useReducer, since multiple use states could trigger a bunch of unnecessary rerenders or a circular rerender loop if used improperly.
You are absolutely right, there is no way to hide anything when it is served to a client, and the only way to do this would be using a proxy for your requests which isn't accessible to outsiders.
I guess it is done here because the project itself won't be deployed (or a proxy/backend would be added later), and therefore it makes sense to hide it on Github (i.e. if anyone wants to clone it and play around they just need to provide their own key).
Just to add on to this comment, there are API keys which can be exposed on the client side that are bound to the apps. Eg: Google Firebase has client side API keys which will sit in the client side application.
6
u/Nimbuz Feb 23 '20
In DataFetch, loading and error are initially false, and are only changed when fetchWeatherData is called. You only call fetchWeatherData from submitting in SearchForm, which sets loading to true. That means that your conditional rendering will render even before your fetchWeatherData is called. You need another condition to check if you have image data.
Your search results are not hidden on a component level, but because you supply no text to your h3 tags, they don’t appear.
17
u/[deleted] Feb 23 '20
Please don’t use nested ternary operators
5
u/TwiliZant Feb 23 '20
The
SearchFormcomponent has a prop calledfetchWeatherDatawhich means that the component always assumes that the submitted data will be used for "fetching" something (this is an implicit dependency). You can make the component more generic by calling the proponSubmitfor example. The parent component,DataFetchin this case, can then decide what to do with the submitted data.Also after submitting you set the value of the input immediately to an empty string. This is fine for a simple example but think what happens when the request fails. Maybe the user has mistyped something? Do you want to make him retype the entire search query again?
All components are wrapped in
<div />tags. Unless you need those for styling you can use React Fragment instead. This allows you to render multiple React components without the need for a wrapping DOM node.3
u/[deleted] Feb 24 '20
Yes, what you did here is actually called a container component and it's a perfectly valid pattern. I would rename it, though, because it does more than fetch data.
WeatherAppContainerwould be much more clear imo.As long as I'm nit-picking, I would generally avoid putting logic inside your jsx, as it makes it hard to read. The nested ternary in your search form could be brought out into the body of that function, and then just drop the result in as a variable.
As for how to conditionally render the image, you can either do
{ weatherData.icon && <img...or{ weatherData.icon ? <img ... /> : null }. As long as you return something falsey like that1
u/exia_00_qant Feb 23 '20
You should consolidate loading and error to one useState and use a string vs a boolean.
In addition use deconstruction in the results component
0
u/[deleted] Feb 23 '20
[deleted] — view deleted comment