Add ability to edit events. Relies upon members_api adjust_event_route_and_permissions#10
Add ability to edit events. Relies upon members_api adjust_event_route_and_permissions#10
Conversation
…e_and_permissions
JulienTant
left a comment
JulienTant
left a comment
There was a problem hiding this comment.
Sorry it's not the best quality review as it's made from my phone!
There's a few things to discard and a few thing that will break from my latest changes, I'm not quite sure I get perfectly what's going on with the classes and cooldux but it seems like there's some mix and match between using local component states in place of the redux store. I would feel better with a second review on this
|
|
||
| #RUN npm run build | ||
| RUN npm install | ||
| #RUN npm install |
There was a problem hiding this comment.
Changes should probably be discarded
|
|
||
| echo "Starting up. PORT is $PORT" | ||
| export PATH=$PATH:$(pwd)/node_modules/.bin | ||
| yarn |
There was a problem hiding this comment.
Changes should probably be discarded
| <PrivateRoute exact path="/users/:id" component={UserDetails} /> | ||
| <PrivateRoute exact path="/events" component={Events} /> | ||
| <PrivateRoute exact path="/events/:event_id" component={Event} /> | ||
| <PrivateRoute exact path="/events/:event_id/edit" component={EventEdit} /> |
There was a problem hiding this comment.
Upon rebasing from the latest develop you'll notice that this section changed slightly.
| const ev = events.one; | ||
| // let eventList = ''; | ||
| // const ev = this.props.read; | ||
| const { read } = this.props.events; |
There was a problem hiding this comment.
Nit: it would be nice to alias this name to something human friendly. "Read" is kid of a vague name for a variable
| const { id, name, frequency, location, description, start_date, end_date } = await this.props.read(this.props.match.params.event_id); | ||
| this.setState({ id, name, frequency, location, description, start_date, end_date }); | ||
| } | ||
|
|
||
| render() { | ||
| const { events } = this.props; | ||
| // const event = this.props.getOne(1); //this.props.match.params.event_id | ||
| // const { events } = this.props; | ||
| // const event = this.props.read(1); //this.props.match.params.event_id | ||
|
|
||
| let eventList = ''; | ||
| const ev = events.one; | ||
| // let eventList = ''; | ||
| // const ev = this.props.read; | ||
| const { read } = this.props.events; |
There was a problem hiding this comment.
The call made line 42 will eventually populate the read variable line 52, we should not store the data loaded for redux in the component state
| }, | ||
| }; | ||
|
|
||
| const columns = [ |
There was a problem hiding this comment.
Unless I'm missing something this is unused?
| }; | ||
|
|
||
| async componentDidMount(){ | ||
| const { event_id } = this.props.match.params; |
There was a problem hiding this comment.
Any reference to props.match.Paramus will break after the update. Tldr is react router stopped supporting classes and we have to use a hack to get access to the params (on my big update PR, look for withRouter calls.)
| return <div key={e.id} style={baseStyles.card}> | ||
| <h5><Link to={`/events/${e.id}`}>{e.name}</Link></h5> | ||
| {dateRange.full_date_string} ({e.frequency}) <br/> | ||
| {formatDateRange(e.start_date, e.end_date).full_date_string} ({e.frequency}) <br/> |
There was a problem hiding this comment.
Nit: This should probably be memoized to avoid having to execute the same function every time the page is retendered.
| return out; | ||
| } | ||
|
|
||
| export default reducerCombined; |
There was a problem hiding this comment.
Why are we not exporting the reducer anymore?
2 participants
@monteslu please double check my changes, I basically cargo-culted your pages/cert_details and state/certs way of doing things without actually knowing what I'm doing.