PR

[victoramontoya]

No description provided.

[Velsu]

Good job on the sprint challenge. The project looks good, altough there are many places for improvements here. The layout looks similar to design files except the phone part.
There is too much repetition in the code and it does not follow the DRY principles.

What looks good:

• Project in desktop and tablet mode resembles the design files
• Pixels properly converted to rems and percentages
• Good usaged of Flexbox
• All boxes change properly based on resolution

Possible improvements:

• Make Your code more DRY. There is too much repetition for all elements. You do not need to repeat all the rules all the time. Specify a set of rules for one element (example box) and then just update one piece that You want to change, do define all of them again.
• DRY also applied to using Media Queries. You do not need to repeat all the rules, just define pieces You want to change based on screen resolution.
• The phone portion is not done correctly for all the sections. Nav should have 100% width and be in cross-axis. Section elements should take 100% with for top and bottom content.
• Banner image for in desktop mode needs some tweaking with positioning.
• Top and bottom section needs some tweaking with padding and margins.

Please refer to github PR comments for DRY examples.

[Velsu]

It should be 1.8rem not rems.

[Velsu]

This is very very bad. One of crucial rules in CSS and overall in programming is DRY which means Do Not Repeat Yourself. You try to not write similar code more than once. In this example You are repeating all the rules for all the boxes everytime which breaks that rule and You should never do it.

What You should do is to set overall rules for .boxes or .box class and then just change individual colors for box1, box2, box3 etc WITHOUT repeating all these rules.

Example:

.middle-content .boxes .box {
    width: 12.5%;
    height: 10em;
    background: black;
    margin: 2% 2.5%;
    color: white;
    display: flex;
    align-items: center;
    justify-content: center;
}
.middle-content .boxes .box1 {
    background: red;
}

That way You specify rules for all the boxes once and then just change individual boxes background color by updating one rule, no reason to repeat all of them all the time because it is reduntant and bad for performance.

[Velsu]

Another mistake with too much repetition. When using media queries You do not need to repeat all those rules all the time. You should only specify what You want to change and rest of rules will remain unchanged and looked up from the top of the file.

Example:

.middle-content .boxes .box {
    width: 12.5%;
    height: 10em;
    background: black;
}

@media (max-width: 400px) {
.middle-content .boxes .box {
    width: 50%;
}
}

In this example box will still have height 10em and background black when it reaches resolution below 400px, but it's width will change. That way You get rid of repetition and keep Your code DRY.

[Velsu]

rem are much easier to control than em.