Community Feedback

  • VIDIT SHAH has commented on Asif Ali Khan's "Use CSS flexbox and Vanilla Js" solution

    0

    Hey! Great job Code really documented well Few Code Improvements which will help when collaborating with others:

    1- File name: main.css and script.js instead of index.css and index.css 2- Use const for all Document objects(apart from sliders) 3-Avoid var 4-Don't use console.log in the production 5-DRY - Don't repeat yourself Eg pricing.innerHTML = "$" + 8 pageviews.innerHTML = 10+"K" convert this into a function 6-Make use of more string Literals

    Again great Job mate! Learnt a lot of CSS from your code base. Cheers :D!

  • Szymon Rojek has commented on Phyf3's "Chat App CSS Illustration" solution

    0

    Hi Phyf3,

    According to your question, try to reset the CSS styles by adding this selector:

    * {
      margin: 0;
      padding: 0;
      box-sizing: border-box;
    }

    It should help. Let me know.

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

    Greetings :D

  • Szymon Rojek has commented on Bals's "Profile card, mobile first site" solution

    0

    Hi Bals,

    Great job! Looks really good :D

    I have checked mainly your HTML structure, a few tips from me:

    • this is a single page component so you can add the h1 tag, for example Victor Crest;
    • 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;
    • check your HTML issue report and try to fix it;
    • card-footer: instead of it you can use ul > li;
    • generally applying semantic tags is not very easy because there is a chance that we can overuse them. In this solution probably I would do the header tag, and footer but it is not necessary here;
    • the circles are a bit tricky in this challenge: I have used pseudo-elements, position absolute, vw and vh, background url, transform translate and @media. In your solution they are not very stable on different devices, for example Iphone X => check your project by the inspector in your browser.
    • check the footer (should be in the middle of a page).

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

    Greetings :D

  • Szymon Rojek has commented on Raj Mhatre's "Site using HTML and CSS" solution

    0

    Hi Raj,

    Well Done :D

    I have checked mainly your HTML structure, a few tips from me:

    • instead of div main_heading you can also use the header tag;
    • the text two paragraphs can be the the h1 with two span inside (main-heading Reliable, efficient delivery, and sub-heading Powered by Technology);
    • you an use only one h1 on a page;
    • instead of div class="sub_heading" use the p tag;
    • instead of div class="cards_container" use can use the main tag here because this the main part of your component in this challenge;
    • I 'd suggest to check your project by the inspector in your browser: first, go from one column, then tablets: two rows with two boxes each and finally desktop design pattern.

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

    Greetings :D

  • Szymon Rojek has commented on Jose Angel Rey's "Testimonials grid section (mobile first) using HTML and CSS (Grid)" solution

    0

    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. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

  • Szymon Rojek has commented on carlito-jdp's "Single price component with grid" solution

    0

    Hi Carlito,

    Well done:D

    Few tips from me:

    • instead of div class="container" will be better to use the main tag here;
    • inside of this main tag you can use one section tag for these three boxes, which can easily become divs (divs are perfect for generic groupings of content);
    • in the second box paragraph => I'd recommend to use two spans (for the price and per month);
    • what do you think about the link instead of button?
    • in the last box instead of p you can use unordered list and list item: ul > li.

    Design:

    • first box: these two paragraphs below the green text should be separate (check design file);
    • per month can be in the middle of the price;
    • removing the outline from a button it is not a good practice;

    Ps. Don't forget to upvote my comment on here that you find helpful.

    Greetings :D

  • Szymon Rojek has commented on Samuel's "HTML and CSS with Flexbox" solution

    0

    Hi Samprorender,

    Well done :D

    I have checked mainly your HTML structure, a few tips from me:

    • instead of div container you can use the main tag;
    • I think the header doesn't need to be here. I'd recommend using the section tag and inside three divs (here divs are perfect for generic groupings of content);
    • you did great with h1 tag but also there is another way to use it: h1 and inside of it two spans (main-heading: Join our community and sub-heading: 30-day, hassle-free money back guarantee);
    • instead of $29 per month I'd recommend to use the p and inside of it two spans for the price and per month;
    • instead of h4 put the p tag;
    • well done with the link, but also you can add additional attribute title,
    • in the last box instead of p will be better to use ul > li;
    • text per month should be in the middle of the price;
    • something wrong with this effect on :hover (check it out).

    In the end, I'd recommend reading about semantic tags and headings => it is very easy to overuse them.

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

    Greetings :D

  • Szymon Rojek has commented on Danny JeanLouis's "Mobile first site using HTML & CSS" solution

    0

    Hi Danny,

    Well done! :D

    I have checked your HTML structure, a few tips for you:

    • instead of div container maybe you can use the main tag here, inside can be one section with 3 divs => in this example divs are perfect for generic groupings of content;
    • the header tag is not needed here IMO => it is very easy to overuse tags;
    • I don't recommend to use the h1 for price. Just to let you know, you should only use one h1 per page. Using more than one will not result in an error, but using only one is seen as a best practice. => h1 is the most important heading, and tells you what the purpose of the overall page is (generally please read about headings h1-h6). I know this is a small project but consider it as one unique single page example (h1 and h2 in inside first box and h2 another boxes);
    • button => please check this article from the blog CSS tricksA Complete Guide to Links and Buttons;
    • check your project on small devices by inspector in your browser (give a margin);
    • check your HTML issues report above and try to fix it.

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

    Greetings :D

  • Szymon Rojek has commented on William Larry's "Mobile first single-price-grid" solution

    0

    Hi William,

    Well done :D

    I have checked your HTML structure, a few tips from me:

    • instead of you can use the section tag;
    • this is a single page component so you can add h1 tag;
    • in the second card you can use p tag with two spans inside for the price and per month;
    • don't use ID's (you will use them with JS);
    • what do you think about link instead of a button?
    • please use a proper spacing in your HTML structure because at the moment it is quite difficult to read;
    • removing the outline from a button it is a bad practice (accessibility matter);

    @afgaIvan mentioned already: try to use proper font, add border radius. Also check your project by the inspector in your browser on different devices (especially on mobiles => fix margin).

    That's it from me. Ps. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

  • Szymon Rojek has commented on Filip Viktor Hanzlík's "Four card section - scss and grid" solution

    0

    Hi Filip,

    Good job :D

    I have only checked RWD this time, so please have check you project by the inspector in your browser on different devices - play with the size of text, margins, paddings (especially on mobile). Also, I'd recommend to start from one column (like you did on mobile) but then create two rows with two boxes each and finally, get the desktop design pattern. Remove HTML issues from a report above. Check it out.

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

    Greetings :D

  • Szymon Rojek has commented on Shreya Bera's "Testimonial-Grid-Section using HTML5 and CSS3" solution

    1

    Hi Shreya,

    Well done :D

    I have checked your HTML structure, CSS, a few tips for me:

    • every testimonial you can treat as a article with header;
    • I think status (graduate) doesn't have to be the heading;
    • this component we can see as a single page component so also we can add the h1 tag, for example for screen readers => sr-only class with hidden;
    • possibly the blockquote can be also added;
    • I would suggest to use readable classes (descriptive) => If you didn't learn BEM I'd recommend to start learn it;
    • remove styles from CSS and transfer them to the CSS file;
    • in this project it will be better do not leave an empty alt;
    • reset the styles by adding selector * { margin/padding 0 and box-seizing: border-box };
    • don't add height and width to the card (that's a reason why you have to learn flexbox or grid => sometimes we have to add height or width but depends on the project);
    • in this project don't use position relative or absolute;
    • it will be better to start building project from mobile first with one column, then for tablets: create two rows with two boxes each and below the 5th box for the entire length of the container and finally: desktop design pattern;
    • check your HTML issue report (try to six it).

    In the end, @sayide mentioned already about it, but I'd recommend firstly start to learn flexbox and when you will be confident with it start to learn grid (don't rush, it will take some time and it is better to learn slowly but effectively) => then you will be able to create good RWD for mobiles, tablets, desktops. Also, about headings: when you want to start your HTML5 structural elements with a h2 (if h1 has been used on the page of course) and then you have to move down the levels gradually if there are other subheadings.

    Two courses providing by Wes Bos for free:

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

    Greetings :D

  • Szymon Rojek has commented on Tati Muniz's "Responsive Article Preview using Sass, Javascript and HTML5." solution

    1

    Hi Tati,

    Welcome here, well :D

    I have checked mainly your HTML, a few tips from me: @jnnrdn already mentioned a few things but also I have got a few tips for you below:

    • instead of the table center-card I suggest to use the main tag;
    • instead h2 you can use h1 here or the h1 tag add under the main tag with the class sr hidden (also short descriptive text about article);
    • instead of h3 use the p tag => it is very easy to overuse tags;
    • 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;
    • check your accessibility and HTML issues in the report above (you can fix them);
    • reset CSS styles * { margin 0, padding 0, box-sizing: border-box };
    • it is better to start from mobile first (check articles about it);
    • check your responsiveness on different devices by the inspector in your browser (something happened with your text on a smaller vwidth - I think because you have added the height);
    • repository: try to divide it into files and folders because it is currently difficult to navigate and find the right file here

    Important: If you want to improve an accessibility, I'd recommend

    • adding an Id to the share-box;
    • adding aria-controls to the share button, with a value of that ID;
    • adding aria-expanded to the share button with an initial value of false;

    Then in JS on click you can:

    • toggle aria-expanded to true
    • toggle the text of the share buttons aria label value between "share" and "close share options";

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

    Greetings :D

  • Szymon Rojek has commented on Dina's "Single price using grid" solution

    0

    Hi Dina,

    I have checked you HTML, a few tips from me:

    • remove CSS styles and transfer them to the CSS file;
    • instead of the div "container" you can youse the main tag;
    • the tag header is not needed here, in this project it is better to use section for all these three cards (every card can be a div);
    • this is a single page component so you can use the h1 tag and two spans inside (heading-main: Join our community and heading-sub: 30-day, hassle-free money back guarantee) or h1 and below h1, then in the card 2 and 3 add the h2;
    • h1 shouldn't be added to the price and per month, here you can use the p tag and span inside of it);
    • instead of a button I'd recommend using a link;
    • check your project on different devices by the inspector in your browser. You should start from one column (on mobiles), then on tablets you can create the design pattern;
    • start to build this project from the mobile first.

    I can recommend two courses providing by Wes Bos for free:

    That's it from me. Ps. Don't forget to upvote any comments on here that you find helpful.

    Greetings :D

  • ApplePieGiraffe has commented on Ford Ulbata's "Profile Card Component" solution

    1

    Hi, Ford Ulbata! 👋

    Good job on this challenge! 👏

    In addition to SzymonRojek's helpful advice, one small suggestion that I think will improve your solution would be to use the font from the original design to make the text on the page look better. 😉

    Also, using CSS background images to add and position the background SVGs might be a good idea, since that'll keep your markup cleaner and I think makes a little more sense to do for background images such as these.

    Keep coding (and happy coding, too)! 😁

Slack logo

Join our Slack community

Join over 30,000 people taking the challenges, talking about their code, helping each other, and chatting about all things front-end!