So I managed to use bootsrap, sass and react, learned alot with this project. I did however have a bit of an issue once I completed the project. Once completed I ran 'npm run build', then I went on to deploy site on netlify. I made a mistake somwhere. I'm guessing its something based on the directory/ command issue, bc it says netlify also runs that npm run build. (Ultimately I deployed it manually) but any resources to do this the correct way would be very helpful, so do comment them. I'd like to fix, and do things correctly. Feel free to provide any additional feedback, thanks! :)
Rien Coertjens
@riencoertjensAll comments
- @moncadadSubmitted about 1 year ago@riencoertjensPosted about 1 year ago
hi, first of all, for the netlify issue: in your "site configuration" settings there is a setting for "build command" (which should be
npm run build
in your case) and "Publish directory" which would bedist
. You also don't neednode_modules
anddist
folder in your repo, since all the information to generate those is in package.json and config files (add the files to your.gitignore
) overall the code looks good, readable and structured well. Some comments to improve.- general: If you have access to the design file, try to pay attention to the details (font-sizes, colors, shadows, padding, margin) try to get them exactly the same as the design.
- react:
- the
setComponent
function inApp
is not really something that is used for a simple case like this. I would pass a single propcard
to theBasicCard
in the App component (so{setComponent(0)}
would become<BasicCard card={cardData[num]} />) then in the component you can do
props.card.id,
props.card.color`, ... - you can move
const cardData = [...]
out of the component (now it would recreated every time the app rerenders) - (BasicCard.jsx) avoid exporting a component as a default (I could do
import Yadayad from "./components/BasicCard"
), instead you can doexport function BasicCard(props) {}
orexport { BasicCard }
when you use this (called named export) you make sure the same component name is used throughout your project (useful when working with multiple people on a projects)
- the
- sass: generally I prefer to leave an empty line before each new declaration, this gives the code some breathing room. I also prefer scss over sass since it looks a bit more like regular css (that's also why they created it) but that's just personal preference, they're basically the same except the syntax. That being said: scss is used a lot more in the "real world"
.card
is declared inApp.sass
andCard.sass
keep it one file to avoid confusion (best practice is to have all the components specific styling (like bg color, border, shadow, text styles) next to the component (in Card.sass) positioning (like margins) can be in app.sass)- &: read up on the use of
&
in sass, it basically will place the parent declarition instead of the&
eg.header { &__header {} }
will result in.header__header: {}
and can clean up the structure of your styles even more - units: you have a declaration
img { max-width: 5em }
, I wouldn't use em in this case, it can have unexpected results. also try to avoid mixingrem
andpx
, try to use the same unit everywhere (rem is usally preferred)
hope you get anything useful out if this, try not to let it overwhelm you and tackle it step by step (you can create a new commit for each change, it's good practice!)
Marked as helpful0 - @sanju321GHSubmitted about 1 year ago
i got tired so i did newbie project.Brings back memories
@riencoertjensPosted about 1 year agofeedback from a professional point of view: you should try to pay a bit more attention to the details, in the original design the cards don't overlap and are evenly spaced and placed into 3 columns, the requirements of the project also state you should be able to "View the optimal layout for the site depending on their device's screen size" and provides a mobile design. Try to get as close to the design as possible, look at spacing between text elements and text colors
Marked as helpful0 - @CherieRoseSubmitted about 1 year ago
I found this challenge much more difficult than the last and am not confident that my code is as clean and efficient as possible - would love feedback!
@riencoertjensPosted about 1 year agooverall the code looks clean and readable, naming of classes is clear. Some feedback points I had
- think about the box model: consider each element as a rectangular box, padding is used to create space inside the box, margin is used to create space between sibling elements. in this case I would apply a padding to the white box that contains all the information. then use margins on the elements inside this box to create space between the siblings.
- re-usability: consider this task part of a big application, are there elements you would want to reuse (for example the buttons): try to create class names that you can reuse and combine (for example
btn
that create's the shape and general style of the button, then you can havebtn-primary
where you apply the blue background color,btn-secondary
would apply a transparent background that way you can doclass="btn btn-primary"
to create a button and reuse it somewhere else - overall structure: I don't have access to the design file, but generally designs like this have a "rythm", for example all elements within the white box will be spaced the same (about 24px in this one, you could create a
ordersummary-child
class to take care of that spacing) - units: I see you're using
px
,rem
andem
, it's best to stick to eitherrem
orpx
(prefereblyrem
) be careful withem
as it can have unexpected results - class vs id: you're using
class
most of the time, but then for theordersummary
child elements you switched toid
, best to stick to class for styling (either one works, but as a rule of thumb)
Marked as helpful0 - @syarief02Submitted about 1 year ago
i'm not actually sure about some flex properties such as justify-content, align-item, align-content. also i don't know a thing about media query, didn't learn it yet. forgive me if you saw so many unnecessery things inside of it >_<
@riencoertjensPosted about 1 year agothat flex cheat sheet looks clean! my go-to cheat sheet for flexbox is (was?) the one from css-tricks: https://css-tricks.com/snippets/css/a-guide-to-flexbox/
Marked as helpful1