@mickyginger
Posted
Hey @tkulic this is great!
I would recommend setting a max-width on the main containing div element in App.js so that the content doesn't fill the entire width of the screen, something like 1200px is reasonable.
I would also consider using a main or section rather than a div as the main containing element, as it's more semantic.
Your components are nicely organised. You can name any file index.js or index.jsx and import it using the name of the folder, so for example:
import Main from './components/Main' would work fine if the component was located in ./components/Main/index.jsx.
Finally I notice that you used a mixture of double quotes " and single quotes ' in your JavaScript. It's probably worth using a formatter like Prettier that will format your code for you.
Great first project, well done! 🎉
@tkulic
Posted
Thanks, @mickyginger, for reviewing my code and writing out all these suggestions!
- I played around a bit with
max-widthand eventually set it to 1200px. It was a major improvement for such a bit of work. - I solved the
divissue with React Fragment. The components were already semantically structured, but the wrappingdivwas ruining the whole thing, as you noticed. I simply removed it. The "root" div is still there though, due to React's nature. - I decided not to rename components for this project, but i will definitely consider this naming convention in the future. Indeed, it does make it clear which file is the most important one in a folder.
- And the quotes thing, yeah... I prefer using double quotes, without a particular reason. The single quotes are stemming from a React plugin I have installed. And Prettier does not support converting everything into double quotes. So I'm a bit stuck. I haven't made up my mind yet what will I do, so I left it as it is for now 😅.
I hope to read from you on my upcoming project uploads!
