Conversation
Added --fix to package.json and ran to auto-fix errors
Moved function definitions before function calls and changed some named exports to default
|
Alright so I think I correctly made another commit and pushed it onto my branch of the master, which appears to have made it show up here. I didn't change much beyond what we talked about. I changed a couple of named exports to default, but am not convinced I did it right. I get the idea of using default when there's only one export, but I'm not fully understanding how you'd import it if you're removing the name. There were also errors for global-require and no-param-reassign that threw me. The require function is exported, so unsure why that's resulting in the error. And for the no-param-reassign I couldn't gather where "registration" was being reassigned, but I think that's mostly because I've gotta re-familiarize with the arrow syntax for functions. |
noahjb
left a comment
noahjb
left a comment
There was a problem hiding this comment.
For the most part, looking good @gallickc. Like you mentioned, we should sync up about module export/import syntax - it can definitely be confusing. I made a few other suggestions throughout the PR too that you're welcome to make changes and push to this branch as you see fit.
| } | ||
|
|
||
| let url = "/community/" + props.groupId | ||
| const url = `/community/${ props.groupId}` |
There was a problem hiding this comment.
For this one, I would update the spaces in the string to something like this:
| const url = `/community/${ props.groupId}` | |
| const url = `/community/${props.groupId}` |
Makes it a tad more readable that way.
| } if (error) { | ||
| return <p>An unexpected error occurred, please come back later</p> | ||
| } | ||
| if (loading) { |
There was a problem hiding this comment.
Is this if (loading)... code duplicated? If so, may be worth cleaning up while you're making changes here.
There was a problem hiding this comment.
FYI, this pattern will most likely change.
| className={"btn btn-link btn-lg"} | ||
| onClick={() => {props.history.push('/community/' + data.thread.group.id)}}> | ||
| className="btn btn-link btn-lg" | ||
| onClick={() => {props.history.push(`/community/${ data.thread.group.id}`)}}> |
There was a problem hiding this comment.
Same suggestion here in terms of readability of the string:
| onClick={() => {props.history.push(`/community/${ data.thread.group.id}`)}}> | |
| onClick={() => {props.history.push(`/community/${data.thread.group.id}`)}}> |
There was a problem hiding this comment.
We will we using an internal <Button /> component for this in the future
| className={"btn btn-outline-primary"} | ||
| onClick={() => {props.history.push('/thread/' + props.threadId)}}> | ||
| className="btn btn-outline-primary" | ||
| onClick={() => {props.history.push(`/thread/${ props.threadId}`)}}> |
There was a problem hiding this comment.
And here:
| onClick={() => {props.history.push(`/thread/${ props.threadId}`)}}> | |
| onClick={() => {props.history.push(`/thread/${props.threadId}`)}}> |
There was a problem hiding this comment.
We will we using an internal component for this in the future
| @@ -1 +1 @@ | |||
| export const configuration = require ('./configuration.json'); | |||
| export default require ('./configuration.json'); | |||
There was a problem hiding this comment.
Let's sync up about module syntax and exports/imports and figure out how we want to do this one. There may be more to this than simply switching it to be default instead of named.
There was a problem hiding this comment.
I would like to talk module export/import syntax as well. I don't understand the tradeoffs of the different options and what is best practice.
| @@ -1,4 +1,4 @@ | |||
| export const errorHandler = (error, history) => { | |||
| export default (error, history) => { | |||
There was a problem hiding this comment.
Let's talk about this one too - way may need to update this (or consuming code).
There was a problem hiding this comment.
Would also like to discuss this. I think we should think about a different paradigm concerning error handling, I see a lot of duplication. A wrapper function / custom hook may be a good solution.
There was a problem hiding this comment.
Agreed. Error handling needs some love.
4 participants
Added --fix to package.json and ran to auto-fix errors