Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

All comments

  • denise 230

    @moncadad

    Submitted

    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! :)

    @riencoertjens

    Posted

    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 be dist. You also don't need node_modules and dist 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 in App is not really something that is used for a simple case like this. I would pass a single prop card to the BasicCard 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 do export function BasicCard(props) {} or export { 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)
    • 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 in App.sass and Card.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 mixing rem and px, 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 helpful

    0
  • @riencoertjens

    Posted

    feedback 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 helpful

    0
  • @riencoertjens

    Posted

    overall 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 have btn-primary where you apply the blue background color, btn-secondary would apply a transparent background that way you can do class="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 and em, it's best to stick to either rem or px (preferebly rem) be careful with em as it can have unexpected results
    • class vs id: you're using class most of the time, but then for the ordersummary child elements you switched to id, best to stick to class for styling (either one works, but as a rule of thumb)

    Marked as helpful

    0
  • @syarief02

    Submitted

    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 >_<

    @riencoertjens

    Posted

    that 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 helpful

    1