Creating the countdown in the header #53
Conversation
|
Deploy preview for react-finland processing. Built with commit f5bf913 https://app.netlify.com/sites/react-finland/deploys/5a9955e6fd0efa40570f3a64 |
|
That's a great start! Check @manpenaloza @okonet Can you provide some guidance on the visual outlook? Preview here. |
|
Inspiration: https://dribbble.com/tags/countdown_timer . Something along https://dribbble.com/shots/3825256-Daily-UI-challenge-014-Countdown-timer would look cool perhaps. I would do it horizontal like that but with tuned style. |
|
nice one! Horizontal look could be a good choice, wireframed sth for further implementation. what do you think? |
|
@manpenaloza That looks great to me! We can use the font we already have. The layout is just 👍 . |
|
@manpenaloza Looks good! @bebraw What steps should be taken now? |
|
@Scoombe Would you be comfortable porting to the proposed layout? You can also try |
|
@bebraw I would be, what's the best way to do this? Inline css in the react component or adding to the existing css files? I might not get time to do this until Friday if that's ok. |
|
@Scoombe No rush. Maybe separate CSS like for now. I might integrate a css-in-js solution (Emotion) later. |
components/Countdown.js
Outdated
| @@ -0,0 +1,39 @@ | |||
| var React = require("react"); | |||
components/Countdown.js
Outdated
| super(props); | ||
| this.tick = this.tick.bind(this); | ||
| this.state = { date: props.initialDate, now: Date.now() }; | ||
| this.TimeDiff = this.timeDiff(); |
There was a problem hiding this comment.
Can these 2 be renamed into something more destinguishable and readable. Also, please don’t use CamelCase for instance variables.
components/Countdown.js
Outdated
| } | ||
| tick() { | ||
| this.setState({ now: Date.now() }); | ||
| this.TimeDiff = this.timeDiff(); |
There was a problem hiding this comment.
Why does it needs to be stored in the instance variable BTW? It looks like a state since you’re rendering those values.
components/Header.js
Outdated
| @@ -4,7 +4,7 @@ import Link from "./Link"; | |||
| import Navigation from "./Navigation"; | |||
| import logo from "assets/img/logo.svg"; | |||
| import navigationPages from "./navigation-pages"; | |||
|
|
|||
| import Countdown from "./Countdown"; | |||
| const Header = ({ pathname, title }) => { | |||
components/Countdown.js
Outdated
| return ( | ||
| <div> | ||
| <p> | ||
| {this.TimeDiff.days} Days  |
There was a problem hiding this comment.
Storing deriving values is an anti-pattern. You could just call the timediff in the render method instead.
|
Can we move the counter below the tagline? |
|
@bebraw @manpenaloza I have made some changes, but I need some help with the css. Could you help with this. Thanks. |
components/Countdown.js
Outdated
| this.updateCountdown = this.updateCountdown.bind(this); | ||
| this.dateToCountDownTo = props.initialDate; | ||
| this.currentDate = Date.now(); | ||
| this.Difference = this.getDifferenceInDates(); |
There was a problem hiding this comment.
What's the point for storing those values as an instance variable? Let's simplify a bit by moving getting all derived values in the render method:
render() {
const { days, hours, minutes, seconds } = timediff(this.state.currentDate, this.props.initialDate, "D:H:m:S");
...
}and remove all instance variables. The only thing you'll need to store in the state is the currentDate:
constructor(super) {
super();
this.state = {
currentDate: Date.now()
};
}
components/Countdown.js
Outdated
| } | ||
|
|
||
| componentDidMount() { | ||
| setInterval(this.updateCountdown, 1000); |
There was a problem hiding this comment.
You forgot to clean up the interval in the componentWillUnmount. This will cause a memory leak...
components/Countdown.js
Outdated
| updateCountdown() { | ||
| this.currentDate = Date.now(); | ||
| this.Difference = this.getDifferenceInDates(); | ||
| this.updateState(); |
There was a problem hiding this comment.
So, here, instead of all this you could just this.setState({ currentDate: Date.now() }). This would trigger re-render.
components/Countdown.js
Outdated
| this.updateState(); | ||
| } | ||
|
|
||
| updateState() { |
components/Countdown.js
Outdated
| }); | ||
| } | ||
|
|
||
| getDifferenceInDates() { |
| } | ||
| } | ||
| Countdown.propTypes = { | ||
| initialDate: PropTypes.string, |
There was a problem hiding this comment.
I'd call the initialDate as toDate since it's the countdown and it counts to some specific date.
|
I can help you with CSS after you resolve the comments. |
|
@okonet I have made the changes you have requested. Thanks, for pointing me in the right direction, I was having problems with the concept of when react was calling the reload. |
|
Merged! Thanks a lot for your contribution @Scoombe. Are you coming to the conference? |
|
Nice work! |
I have had a go at addressing issue #26. I need some pointers on the style of the change, also this is my first time looking at react so any help there would be nice.