Skip to content
OverviewSolutions
5
Comments
174
Szymon Rojek
@SzymonRojek

All comments

  • Astrid•100
    @astridvSubmitted almost 4 years ago

    Hi guys! 😄

    This is my first time trying out BEM so extra interested in getting some feedback on that ✨ I really liked it at first as I didn't have to worry about what to name my classes, but I also got confused by some aspects of it; like how do you deal with margins if you aren't supposed to place it inside blocks? 🤷‍♀️ I also ended up with some excessively long class names for my desktop styles 😅 For reference, I tried to follow the BEM convention as outlined by CSS Tricks in this article.

    Edit: additional question after I saw the HTML validation that is complaining about the lack of headings in my sections and article 🙈 I don't think it makes sense to have headings inside each section in this case (but please let me know if you think otherwise!). So would it be better to just divs instead of sections in this case?

    Any general feedback on the solution is, of course, also very appreciated 😊

    Responsive grid testimonials using flexbox, CSS grid and BEM

    3
    Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    @steventoben and @astridv

    Well done explanation @steventoben. Yes it is not a good practice to put a section inside of an article. To be honest for me it is always hard to implement an article and section because it is very easy to overuse these semantic tags.

    Article elements are good for content that could stand alone by itself. For example: blog posts, news articles, forum posts. What's about section: this element we can use for larger groupings of related content. For example, the feedback section on the solution page.

    In my opinion it is not a mistake when each of testimonial will be an article and inside of it will add additional header, blockquote, heading, paragraphs, img. If we see this project as a single page component then we can add the main tag, section, h1 with the class sr-only set to hidden for the Screen Readers, then name of the person can become the h2, rest of the texts will become paragraphs. All depends on the context, content etc. Also, bold text doesn't have to indicate on heading. What do you think?

    From the other hand possibly we can use a section here just for groupings of related content (wrapper). Also, inside of a section we can add 5 divs (each testimonial). I agree with you that divs are perfect for generic groupings of content.

    @steventoben I like your comments and the way how you did explain all details.

    Ps. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Catuva21,

    Well done with RWD :D

    A few tips below:

    • in my opinion this div class="centro" doesn't have to be here;
    • instead of div class="container" you can use the main tag;
    • instead of section class="section" probably you can use an article;
    • alt text => don't need to use words like picture or image, photo, icons in the alt text as it's already announced as being an image, so you can easily type a person's name;
    • instead of h2 for the text "Verified Graduate" I'd suggest to use the p tag;
    • also, this is a single page component so the h1 can use with the class sr-only set to hidden visible only for the Screen Readers (we have to use headings gradually and do not skip them => outline algorithm);
    • instead of the h3 I'd suggest using the p tag because it is not a heading => bold text doesn't have to indicate on heading;
    • classes are not readable and descriptive so I'd recommend learning BEM naming convention;
    • max-width should have 1440px;
    • check the img border color (according to the project).

    Keep coding :D

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Catuva21,

    Well done :D

    A few tips below:

    • instead of div class="container" you can use the main tag;
    • I think you don't need this div class="bloque";
    • instead of h2 and h3 in the first box I'd suggest to create one the h1 tag with two spans inside of it => main-heading: Join our community and sub-heading: 30-day, hassle-free money back guarantee;
    • in the 2 and 3 box instead of the h3 you can add the h2 tag (we have to use headings gradually and do not skip them);
    • the p doesn't have to be added to the a tag;
    • in the third box do not use one p tag and br inside of it => the best idea will be ul > li (please read about br tag);
    • text "per month" should be in the middle of the price;
    • will be nice to add :hover state to the link;
    • these to sentences in the 1 box (grey color) should be separated when the desktop patter comes up;
    • add max-width;
    • also you have used an explicit height: that's not a good practice especially that you want to use the flexbox or grid. It is essential to understand well the height and width vs min-height/max-height & min-width/max-width. You shouldn’t need to give items height unless they have a background or absolutely-positioned or floated elements within them that would not normally be accounted for in the height of an element. Experienced developers use min-height and min/max-width more than anything else. It allows elements to grow and shrink.

    Keep coding :D

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Vaishnav,

    Well done :D

    A few tips below:

    • instead of div class="header", use the header tag;
    • we can use only one the h1 on a page, in this project you can create one h1 with two spans inside main-heading: Reliable, efficient delivery and sub-heading: Powered by Technology;
    • instead of div class="container" you can use the main tag just to indicate the main content of a page;
    • alt text => In this project, images have an only a decorative role - that's a reason why alt text should be provided as an empty (alt="") so img can be ignored by assistive technologies, such as screen readers;
    • div class="card-icon", div class="card card-four" are unnecessary;
    • check a project in your browser by the inspector on different devices => what's about the tablet breakpoint (two rows with two boxes each). In my opinion one column till 1200px is too long, check it out;
    • add max-width to the container;

    Ps. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    0
  • Tazzz•60
    @ta-zzzSubmitted almost 4 years ago

    Hello everyone :>

    Here I tried using the BEM naming convention for the first time. So I'm not so sure if I did it correctly. Feedback is highly appreciated!

    What I used:

    • Semantic HTML
    • CSS Grid
    • CSS Flexbox
    • BEM Naming Convention
    • Responsive design

    Responsive testimonials grid section using BEM naming convention

    1
    Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Ta-zzz,

    Well done with RWD :D

    A few tips below:

    • check HTML issues in a report above and try to fix it;
    • instead of section class="card-cntnr" just use the main tag;
    • alt text: picture of daniel clifford => don't need to use words like picture, image, photo, icons in the alt text as it's already announced as being an image. You can just simply type a name of this person, for example: alt="Daniel";
    • for the text Verified Graduate don't need to use the h2 tag, instead of it just use the p tag;
    • also, this is a single page component so the h1 can use with the class sr-only set to hidden visible only for the Screen Readers (we have to use headings gradually and do not skip them => outline algorithm);
    • don't need to use the h3 tag, instead of it just the p will be ok => bold text doesn't have to indicate on heading;
    • check the design pattern: the quote pattern in the 1 box should be under the text;
    • I don't know why you have used width / height auto => also, add max-width to the container.

    Keep coding :D

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Vaishnav,

    Wellcome here, well done :D

    A few tips below:

    • check your HTML report issues above and try to fix it;
    • instead of div class="container" you can use the main tag;
    • instead of the h2 I'd suggest using the p tag because it is not a heading => bold text doesn't have to indicate on heading;
    • this is a single page component so the h1 can use with the class sr-only set to hidden visible only for the Screen Readers;
    • alt text should be added => just write name of a person because these images don't have a decorative role;
    • I'd suggest learning the BEM naming convention, especially that you have used the grid;
    • RWD: check a project in your browser by suing the inspector on different devices => add margin top and bottom; also, you can create one @media breakpoint for tablets by adding another design pattern: two rows with two boxes each and below the 5th box (named Kira) along the length of the entire container (otherwise all content looks very tight);
    • I think you should add max-width 1440px, also in my opinion max-width 600px doesn't have to be added to the cards;
    • add border to the img (check the design pattern).

    Keep coding :D

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    2
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Victor,

    Well done :D

    A few tips from me below:

    • instead of this div class="container" use just the main tag;
    • creating clickable element by div it is not a good practice (accessibility) => use the tags dedicated to it like a link or button but in this project I'd suggest to use a link, why? check this article from the blog CSS-tricks: A Complete Guide to Links and Buttons;
    • names of classes are not readable and descriptive I'd recommend using BEM naming convention;
    • text "per month" is not in the middle of the price;
    • check your project by the inspector in your browser on different devices => it doesn't look well, margin, button, layout etc.
    • the same problem with the width and height (check my comment in four cards feature section challenge).
    • try to start building your project from the mobile first.

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Andy Curley•30
    @racurleySubmitted almost 4 years ago

    Not perfect or the most elegant but I think I figured it out. Tried doing this with not grids and that was not going to happen so I used a display: grid; (not a flexgrid). Fiddly on the positioning.

    I never did get the "per month" to center align vertically. /* update */ Applied the feedback I had received (Big Thanks!) and corrected some issues.

    • Dropped unnecessary Div's
    • Changed how I switched sizing and padding based on screen size. Mobile looks right I think.
    • Got the Pricing line to vertically align
    • Changed the Why Us list to a UL/LI
    • General clean up
    • Did not change to BEM naming convention but am looking at how it works

    Single Price Grid Component - HTML and CSS

    3
    Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Andy,

    Welcome here, well done :D

    I have checked your project, mainly HTML structure and RWD by the inspector in my browser, a few tips below:

    • instead of section class="card-container" you can use the main tag just to indicate the main content on a page;
    • don't have to create that kind of classes like padding-small, padding-large;
    • generally I'd recommend learning about BEM naming convention and how to create readable and descriptive classes => in the future you will get lots of gratifications from it;
    • if you will use the grid you don't need this div class="card-subscription";
    • "per month" in the middle of the price: don't have to use two spans inside the p, one will be enough. In my project I have used two paragraphs but now I think it will be better to use just a one span inside of the paragraph;
    • well done with the link (lots of people used a button but in my opinion the link has got a better reason to be here), anyway check this article from the blog CSS-tricks:A Complete Guide to Links and Buttons;
    • third box: instead of many p it will be better to use ul > li;
    • reset the CSS styles: * { margin: 0; padding: 0; box-seizing: border-box; };
    • you gave explicit width on mobile size: that's not a good practice especially that you want to use the flexbox or grid. It is essential to understand well the height and width vs min-height/max-height & min-width/max-width. You shouldn’t need to give items height unless they have a background or absolutely-positioned or floated elements within them that would not normally be accounted for in the height of an element. Experienced developers use min-height and min/max-width more than anything else. It allows elements to grow and shrink;
    • check a project i your browser on different devices by using the inspector => this is a good practice to see the project at the end, now there is a problem with a margin an generally project doesn't look well on mobiles.

    That's from me.

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    3
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Jose,

    Welcome here. Well done! :D

    I have only checked your HTML structure by inspecting the page in my browser, a few tips for you:

    • I would give h1 as the main tag for the Screen Readers with the class -sr-only set to hidden, then h2 only for a person name and status (graduate) as a paragraph but of course your solution is also fine (it is a bit tricky to decide which h1-h6 we have to choose). We can also have h1 and 2 x span tag inside of it (I think it is better). Check this article from the blog CSS-tricksHTML for Subheadings and Headings;
    • the bold text: IMO it shouldn't be treat as h2. This text is a description and looks like an intro but doesn't have to be the heading. A few weeks ago I would do it like you but now I have changed my point of view about it. It is very easy to overuse semantic tags;
    • don't need title attribute in the img tag;
    • RWD looks great but IMO lots of thing are happening and the text is flickering, moving a bit. I think that kind of effect is nice but it is hard also to read :D anyway, congrats for an invention;

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    **Hi Victor, **

    Well done :D

    I have checked your HTML structure and RWD by the inspector in my browser, a few tips below:

    • instead of div class="text" I'd suggest to use the header tag;
    • instead of div class="cardsContainer" I would suggest to use the ** tag** just to indicate the main content on a page;
    • you have used so many nested wrappers divs and this is unnecessary => try to simplify it;
    • instead of the h3 it will be better to use the h2 (we can not skip the headings);
    • alt text => In this project, images have an only a decorative role - that's a reason why alt text should be provided as an empty (alt="") so img can be ignored by assistive technologies, such as screen readers;
    • don't have to use ID's here => that's a reason why we have got classes. I'd suggest learning BEM naming convention;
    • RWD: check a project by using the inspector in your browser on different devices, especially tablets => from 550px till 820px (approx) the layout doesn't look well. You can start from one column, then create two rows with two boxes each and finally get the design desktop pattern;
    • you have used an explicit width and height (container and cards) => important: that's not a good practice especially that you want to use the flexbox or grid. It is essential to understand well the height and width vs min-height/max-height & min-width/max-width. You shouldn’t need to give items height unless they have a background or absolutely-positioned or floated elements within them that would not normally be accounted for in the height of an element. Experienced developers use min-height and min/max-width more than anything else. It allows elements to grow and shrink.

    I'd recommend reading more about semantic tags, headings, alt, width and height when using flexbox && grid.

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    2
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Victor again,

    Another challenge, well done :D

    I have checked your another project Four card feature section so please have a look because lots of points can be included here:

    • you have used so many nested wrappers divs and this is unnecessary => try to simplify it;
    • don't have to use ID's here => that's a reason why we have got classes. I'd suggest learning BEM naming convention in English as a universal language for programmers;
    • this is a single page component so you can add the h1, for example with the class sr-only hidden for the Screen Readers, then instead of the h3 use the h2 because we can not skip the headings;
    • RWD: check a project by using the inspector in your browser on different devices, especially on tablets, these cards could have a different pattern: create two rows with two boxes and below the fifth (named Kira) could be along the length of the entire container;
    • the same problem with the height and width (check Four card feature section project).

    I'd recommend reading more about semantic tags, headings, width and height when using flexbox && grid.

    Ps. Please, don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1
  • Szymon Rojek•4,540
    @SzymonRojekPosted almost 4 years ago

    Hi Sarvesh,

    Well done, I have used last time the tailwind. What do you think about this technology? Do you like it?

    I have checked your HTML structure, a few tips below:

    • check accessibility & HTML issues in a report above and try to fix it;
    • I'd recommend using semantic tags in this project (check it out why they matter), for example: instead of divs using for texts I'd suggest to use the p tag;
    • I'd suggest adding heading;
    • you can add RWD for tablets => start from mobile with one column, then two rows with two boxes each and the 5th box below on the entire length of the container, finally the desktop design pattern.

    Ps. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

    1