-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add Loading animations and update error handling #14
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/julianblohm/hotspotornot/6em5eoswo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work 👍
src/ResultPage.js
Outdated
</section> | ||
<section className="information-wrapper"> | ||
<a href="https://www.bundesregierung.de/breg-de/themen/coronavirus/corona-bundeslaender-1745198"> | ||
<Button text="Regeln der Bundesländer" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work? Actually, a button within anchor-tags is not valid HTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh really? Actually it works, but you're right. Gonna change it to just a link.
src/components/Button.js
Outdated
@@ -1,33 +1,40 @@ | |||
import styled from 'styled-components/macro' | |||
import PropTypes from 'prop-types' | |||
|
|||
function Button({text, onClick}) { | |||
|
|||
function Button({ text, onClick, className }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component name sounds like a button element, but it represents a div.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah true, did some stupid stuff there, gonna remove the div and solve the styling with props.
<ErrorPageStyled> | ||
<section className="county-class-red result-wrapper"> | ||
<h3>Sorry, Daten konnten nicht geladen werden.</h3> | ||
<span>Probiere eine neue Suche!</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to set the styling in "result-wrapper" class to get rid of the span
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but have to stick with the span, cause don't want the whole text in the result wrapper being font-weight: 300.
animation: loading-spinner 1s cubic-bezier(0, 0.2, 0.8, 1) infinite; | ||
} | ||
|
||
.loading-spinner div:nth-child(2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting selector :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy-pasta 🍝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really well structured and clean code. I left only three comments. Keep it up!
src/MainPage.js
Outdated
</MainPageStyled> | ||
isDataLoading={isDataLoading} | ||
setSearchOrigin={setSearchOrigin} | ||
></Form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't embed anything in your form tag. So you can close it directly without a closing tag.
src/ResultPage.js
Outdated
)} | ||
<h3> | ||
Die 7-Tage-Inzidenz <br /> liegt bei{' '} | ||
{countyData.incidence}.{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem with the empty brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I really don't know where they come from 🧐
Update--> found out that prettier formatted the text and placed these brackets as string with space, so that the incidence has space to the text.
src/MainPage.js
Outdated
padding: 10px; | ||
display: grid; | ||
grid-gap: 20px; | ||
background: #f5f5f7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes you work with color variables. So it's not quite stringent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! Got the variables after I styled that one, I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Work! Maybe you could think about your color variable naming. It's only a little think :)
@@ -31,7 +31,7 @@ const MainPageStyled = styled.div` | |||
padding: 10px; | |||
display: grid; | |||
grid-gap: 20px; | |||
background: #f5f5f7; | |||
background: var(--silver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to work with variablen. You might think about it work with main oder primary in the naming. So it's a little bit clearer what kind it is.
} | ||
export default styled.button` | ||
border: none; | ||
background: ${(props) => (props.disabled ? 'var(--gray)' : 'var(--blue)')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here ;-) "more speacking names" maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also easier to change the main colors in the whole app that way. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work and a very important app! Excited to see what is comming next. :) If questions arise regarding my comments, feel free to ask. I hope it is helpful.
src/MainPage.js
Outdated
@@ -22,7 +22,7 @@ function MainPage({ | |||
errorMessage={errorMessage} | |||
isDataLoading={isDataLoading} | |||
setSearchOrigin={setSearchOrigin} | |||
></Form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be there a more descriptive name for the form and what it is doing? Hard to think of something, but maybe you have a practical idea. CheckHotspotForm
or something like this?
} | ||
export default styled.button` | ||
border: none; | ||
background: ${(props) => (props.disabled ? 'var(--gray)' : 'var(--blue)')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also easier to change the main colors in the whole app that way. :)
font-size: 1rem; | ||
border-radius: 5px; | ||
margin: 20px 0; | ||
cursor: pointer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could think of grouping properties according theire functions to have a better overview. E. g. everything with display, fonts and so on.
border-radius: 5px; | ||
margin: 20px 0; | ||
cursor: pointer; | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same point reagrding grouping here as well :)
New loading animations are displayed during fetching and with the dynamic URL there was a reason to separate the error handling of main and result page. Therefore I added a error page shown if fetching was not successful from result page.
-Prettier fixed