Closed edavidj closed 4 years ago
Thanks for the feedback, @edavidj.
The suggestion for testing also came up in the SME review; I added the configuration in the pom.xml to run "all" the tests (npm test a), where the user can add the configuration if they wish to do so (since testing is not one of the focuses of the guide).
In terms of clarity, I think the last section on testing could tell the user to run the
npm run test
command fromfrontend
just to show how it's actually run. Or if it's already run by dev mode at some point just an explanation would help.
I took out the sentence that was misleading.
In the axios section it's a little misleading to say you need axios (even if it's highly recommended) since you can use the
fetch()
api without additional installation in modern browsers.
Oh not nitpikcy, that's a good suggestion. Most of the material I looked at for our use case were still using class components so I thought it would be ideal to follow that. I would leave it for now and if it comes up again we should surely make the switch.
This is a little nitpicky, but it'd be a good look to adopt React Hooks + functional components rather than the class based one seen in
ArtistTable.js
. This is considered the best practice since 16.8 (but not high priority). It would ultimately require changingcomponentDidMount
to a useEffect hook, and the state variables to useState
Changes verified by Ethan.
Comments:
npm run test
command fromfrontend
just to show how it's actually run. Or if it's already run by dev mode at some point just an explanation would help.fetch()
api without additional installation in modern browsers.ArtistTable.js
. This is considered the best practice since 16.8 (but not high priority). It would ultimately require changingcomponentDidMount
to a useEffect hook, and the state variables to useState