create HTML page using blocks previous created #4164

MatheusStefan · 2024-02-26T19:12:30Z

I had some troubles with the previous task, so I made a new one:

DEMO LINK
TEST REPORT LINK
All component follow BEM and use SCSS
repeated sizes and special colors are put to variables
Grid is used for the columns
cards are shown in 1, 2, 3 or 4 columns based on screen resolution
All changes on :hover are smooth
Code follows all the Code Style Rules ❗️

jv-aquino

Your code is good overall and very organized!! You are improving a lot Matheus, and I hope you forgive me for being a little harsh sometimes but I'm only that way because you are a great student and I trust that you can make mind-blowing solutions!!

src/index.html

jv-aquino · 2024-02-26T19:51:20Z

src/index.html

+    </header>
+
+    <main
+      class="container"


In general a container is not counted as a block, just some block of html code, so in this case it's fine

Yeah, I just made like that because it's a requirement "- use <main> tag for cards container". Some requirements are awful..

Also, don't worry about the extensive feedbacks, I really want to improve my skills and every detail it's very important to me. Thank you so much!

src/index.html

src/styles/card.scss

src/styles/header.scss

…alog into develop

pedro-ruas

Some minor issues that prevent the expected behavior accordingly to figma design. I believe you can fix them all quite easily ;)

pedro-ruas · 2024-03-08T19:13:37Z

src/index.html

+            >
+              Apple
+            </a>
+          </li>


Matheus, you could add an empty line here and between other sibling elements in this file, check Code Styles Rules

pedro-ruas · 2024-03-08T19:15:09Z

src/index.html

+            Buy
+          </a>
+        </div>
+      </div>


An empty line here would be good also, for the same reason of my previous comment, these 2 cards are siblings

pedro-ruas · 2024-03-08T19:24:51Z

src/styles/stars.scss

I believe you finished this task before we review the others Stars and Product Card.
The approach you've used there is a better implementation for the stars styling for 2 main reasons:

Here you have more code repetition, you could declare all stars with same styling using a different synthax (stars--0, stars--1...)

Your declaration here is applying a fixed style for all stars reviews, you see, combining your BEM naming on the stars declaration in HTML with this styling here, all Product Cards will have 4 active stars, even if we change the class in HTML. Ideally, if we changed HTML class to, let's say stars__star--2, only 2 stars would be active, not 4, but because you are declaring only the last-child to be not-active, you always get 4 stars on the card, no matter the actual review count.

**Not sure if my explanation was clear, but you can ask me on chat if you want

pedro-ruas · 2024-03-08T19:29:45Z

src/styles/mixins.scss

@@ -0,0 +1,17 @@
+@mixin two-columns {


I like your use of mixins, but I'd like to suggest a rename for them to allow them to be used more often, which is the purpose of a mixin (imagining this is part of a bigger project).

So, the way I see it, two-columns is used for small-screens or mobile, three-columns is used for medium-screens* or tablets and four-columns is used for large-screens or desktop.

Giving them a name as mentioned makes it possible to re-use them in the future for other purposes other than only the column count, imagine that you wanted to change the background color of your page based on screen size too, in this way, the name would remain meaningful for all usage.

pedro-ruas · 2024-03-08T19:35:15Z

src/styles/card.scss

+  justify-content: center;
+
+  @include two-columns {
+    grid-template-columns: repeat(auto-fit, 200px);


And here we have a problem, actually 2:

You are using auto-fit which auto calculates the column count based on element width. In this case, as the element is the whole page itself, and you are applying it for screens of max 488px, it ends up returning 2 columns (488 / 200 = 2,...). However, if let's say, you wanted 2 columns on a bigger screen, this would not be the case. And if a screen is 630px wide, it would result in 3 columns instead of 2, which is also not a desired behavior here.

Second point is that you are using the same declaration for all mixins that represent screen size, so either there is no need to use the mixins, or the style declaration should be different for all mixins. Can you guess which option is the correct one? :)

**There is a hint in this image:

I just checked. It says I'm 1 commit behind, but just commited an update and now it's working great!

EdPirro

Great effort, @MatheusStefan! You're almost there, just a few touch-ups and your solution should be perfect! 👏

Also, your deployment seems to be outdated, please make sure to deploy the latest version for review

EdPirro · 2024-03-11T23:05:38Z

src/styles/card.scss

+  @include on-mobile {
+    grid-template-columns: repeat(2, 200px);
+  }
+
+  @include on-tablet {
+    grid-template-columns: repeat(3, 200px);
+  }
+
+  @include on-desktop {
+    grid-template-columns: repeat(4, 200px);
+  }


Your deployment does not seem to be up to date with your code:

Deployed site:

Gonna check this, thank's!

src/styles/card.scss

src/styles/header.scss

…alog into develop

EdPirro

Great job, @MatheusStefan! Sorry, I overlooked some issues in my last review, and I'll have to ask you to fix them 😬. If you have any questions, please let me know

EdPirro · 2024-03-12T22:11:40Z

src/index.html

+      </nav>
+    </header>
+
+    <aside


I don't see why you have this aside here. Also, your deployment may be outdated again as it doesn't show in your deployed html:

I used an aside tag to do a menu when on mobile. I know it's not a requeriment, but I wanted to be completely responsive, if that's okay(?). If not I'll remove it.

Also, I talked with Pedro about the deploy and he said it's working fine. Maybe your cache was deleted when testing it?

src/index.html

src/styles/card.scss

pedro-ruas

Awesome! Great implementation and everything works as expected.
I really liked your approach for the menu, if I could, I'd give some extra-points for that.
Only comment is: your declarations from line 17~21 at stars.css could have been done using a @for. But all great!

MatheusStefan added 2 commits February 26, 2024 16:04

create HTML page using blocks previous created

95a1c97

sync remote with local

b86ce09

jv-aquino suggested changes Feb 26, 2024

View reviewed changes

MatheusStefan and others added 5 commits February 27, 2024 10:18

fix css and html + syntax issues

cda53bc

Merge branch 'master' into develop

ba96917

update package.json version

5dd75d4

Merge branch 'develop' of https://github.com/MatheusStefan/layout_cat…

a7d063a

…alog into develop

update package version

7d33706

MatheusStefan requested a review from jv-aquino March 5, 2024 15:01

Merge branch 'mate-academy:master' into develop

98deadc

pedro-ruas suggested changes Mar 8, 2024

View reviewed changes

MatheusStefan and others added 2 commits March 11, 2024 15:09

fix BEM syntax + layout_stars and indentation

d1271e7

Merge branch 'master' into develop

64938a8

MatheusStefan requested a review from pedro-ruas March 11, 2024 20:59

EdPirro suggested changes Mar 11, 2024

View reviewed changes

MatheusStefan added 2 commits March 11, 2024 21:03

fix BEM syntax + gh-pages deploy

107177e

Merge branch 'develop' of https://github.com/MatheusStefan/layout_cat…

1768824

…alog into develop

MatheusStefan requested a review from EdPirro March 12, 2024 00:15

implement responsive menu

5bdadfa

EdPirro suggested changes Mar 12, 2024

View reviewed changes

MatheusStefan added 3 commits March 13, 2024 09:22

fix blocks and hover style

6e8e1e4

fix demo link and test report link

23e818f

fix problems

1d88b77

MatheusStefan requested a review from EdPirro March 13, 2024 18:33

pedro-ruas approved these changes Mar 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create HTML page using blocks previous created #4164

create HTML page using blocks previous created #4164

MatheusStefan commented Feb 26, 2024

jv-aquino left a comment

jv-aquino Feb 26, 2024

MatheusStefan Feb 27, 2024

MatheusStefan Feb 27, 2024

pedro-ruas left a comment

pedro-ruas Mar 8, 2024

pedro-ruas Mar 8, 2024

pedro-ruas Mar 8, 2024

pedro-ruas Mar 8, 2024

pedro-ruas Mar 8, 2024

MatheusStefan Mar 12, 2024

EdPirro left a comment

EdPirro Mar 11, 2024

MatheusStefan Mar 11, 2024

EdPirro left a comment

EdPirro Mar 12, 2024

MatheusStefan Mar 12, 2024

MatheusStefan Mar 13, 2024

pedro-ruas left a comment

create HTML page using blocks previous created #4164

Are you sure you want to change the base?

create HTML page using blocks previous created #4164

Conversation

MatheusStefan commented Feb 26, 2024

jv-aquino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-ruas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdPirro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdPirro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-ruas left a comment

Choose a reason for hiding this comment