Skip to content
OverviewSolutions
21
Comments
48
P
Rupali
@rupali317

All comments

  • P
    Rupali•890
    @rupali317

    Hello @Manith112

    Great work on the challenge. The animation on it gives a wow factor to your work

    I have the following feedback:

    • Your solution currently has a grid arrangement for desktop and mobile. It does not have the grid arrangement for tablet screens.
    • Fonts should be in rem instead of px since the latter is bad for accessibility
    • The mobile version does not need to use a grid since, performance-wise, a grid is more expensive than a flex. Since the cards are arranged in a column, using flex is a suitable approach.
    • The headings inside the card should not be <h1>. It should be <h2> since we are not supposed to skip headings and can only have 1 <h1> in a page per accessibility guidelines
    • You should improve the colour contrast of the grey text since it appears light and affects the readability
    • In your CSS, you have used float:right for the icons. You should use flexbox since float:right is not considered a good practice in terms of layout consistency and accessibility
    • For the header, you have used 2 instances of <h1>. Actually just 1 <h1> is enough. You can have the styling effect by using ::first-line.
    • You can make your HTML more semantic by using HTML elements like <header> and <main>
    <body>
 <header>
  <h1>...heading text...</h1>
  <p>...paragraph text...</p>
 </header>
 <main>
  ...rest of the content...
</main>
    • In your CSS, the CSS variable should be as generic as possible. I would not name it as --cyan, --red. What if the color scheme of your project changes? Then you have to update the variable naiming which causes maintainability issue. Names like --color-1, --color-2 is more suitable
    • In body, you have a CSS definition of margin:60px 0px. Use padding instead in this case because lets say there was a background color on the body. Having a margin would create white spaces. Padding will not have that issue.
    • You can also create CSS variables for padding, font sizes, font weights, margin, border radius, shadows, line height, letter spacing. It is not reserved only for colors.
    • In .box .a h1{color: var(--Very-Dark-Blue)}, .box .a p{width: 90%;padding-bottom: 10px;}, .box .a img{float: right;margin: 10px 0;} you don’t need 90% and the “a” class
    • In your CSS file, the lines from 56 to 84 can be shortened since the former is repetitive. Refer to this:
    .box {
    background-color: white;
    padding: 30px;
    border-radius: 5px;
    border-top: 5px solid var(--Cyan);
}

.box .first {
    box-shadow: 0px 17px 18px 8px rgba(0,0,0,0.1);
    
}
.box .two {
    box-shadow: 0px 17px 18px 8px rgba(0,0,0,0.1);
}
.box .three {
    box-shadow: 0px 17px 18px 8px rgba(0,0,0,0.1);
}
.box .four {
    box-shadow: 0px 17px 18px 8px rgba(0,0,0,0.1);
}

    If you need any clarification, please let me know

  • P
    Rupali•890
    @rupali317

    你好 @sixwordxiong

    今天怎么样?

    Front-end mentor team requested me to review your code. Kindly refer to my feedback.

    1. General feedback: Since you hosted this via Github, it automatically serves index.html as the default page for a directory instead of your exp.html. Why not write the code inside the index.html instead? Because I just see the initial view of the html when I access your url

    2. HTML feedback: a) You do not need <br> in the headings because the number of lines for headings varies in mobile and desktop. A CSS styling of text-wrap:balance can help achieve the look and it will work for desktop view and mobile view. b) the <img> tag implementation that you used will work only for desktop. In order for it work, you can explore <picture> element.

    3. CSS feedback: a) You should never use px unit for font sizes because it is bad for scalability (some users rely on bigger font sizes by altering the font setting in their browser). px unit is fixed. Use rem unit as this will scale when users modify the font settings from the browsers b) Your CSS should use CSS reset. Refer to articles such as: Joshua's CSS reset, Andy Bell's CSS reset c) Use CSS custom variables for colors, font size, spacings, font-weights, line heights, letter spacing, border radius, border shadow. Refer to my CSS

    Let me know if the above feedback works. Feel free to ask for any clarification

  • P
    KonradJam•1,140
    @KonradJam
    What challenges did you encounter, and how did you overcome them?

    Most challenge I have with styling list. To be aligned to left side of parent and have space between marker and text.

    What specific areas of your project would you like help with?

    If I made some mistakes, I probably don't know about them. So please look at my solution and be very critical.

    Recipe page solution

    #accessibility
    2
    P
    Rupali•890
    @rupali317

    Hello @KonradJam

    I am peer-reviewing your code as per the request from FrontEndMentor team

    What you did well:

    • You have used rem instead of pixel unit for font-size which is excellent for scaling font sizes if users choose to scale the font settings.
    • You have demonstrated strong knowledge of HTML semantics. For instance, you have used <strong>, <time>.
    • In the mobile view, you have successfully centre aligned the bullets with the text.

    Suggested improvements:

    • The image should have a border-radius, especially in tablet and desktop mode.
    • Moreover, you can improve the loading time and bandwidth usage by resizing the image size. I have noticed that the intrinsic size of your image is 1312px X 600px. In the design the maximum rendered size of the image is only 656px X 300px. Larger resources increase the page load time and increase the bandwidth usage. You can consider resizing the image to 656px X 300px and there will be improvement in your page load. PageSpeed Insights will help you to determine such metrics.
    • Include width and height attributes for image in order to minimize cumulative layout shifts. Moreover, you can consider using webp image format rather than jpeg since the former has a smaller file size with the same image quality.
    <img src="./assets/images/image-omelette.webp" alt="A golden-brown omelette folded with fresh vegetables" width="375" height="171">
    • I noticed that you have used a class recipe-header-title for the <h1> and you have defined all the stylings within that class. In this case, you can just use the <h1> for styling. The class is not needed in this case. Elements have a lower specificity than classes. It is better to aim for lower specificity as much as possible. There is only one <h1> so the class is not necessary. Lower specificity is better for maintaining the styling and help the overall performance.
    h1 {
    font-family: 'Young Serif', serif;
    font-size: 2.25rem;
    font-weight: 400;
    line-height: 2.2rem;
    margin-top: 2.5rem;
    margin-bottom: 1.25rem;
    color: var(--stone-900);
}
    • Your CSS should have CSS reset to provide a clean/consistent slate for the CSS stylings across all the browsers. You can refer to these articles. Joshua's CSS reset, Andy Bell's CSS reset
    • While you have CSS definitions for colors, you should consider CSS definitions for spacing, typography. You can refer to my CSS definition
    • You are fetching the font families via a link so far. With this approach, you have to rely on this external link to always be available. Another approach is you can consider downloading those resources in your project and load them via the CSS styling
    @font-face {
  font-family: "YoungSerif";
  src: url("./assets/fonts/young-serif/YoungSerif-Regular.ttf")
    format("truetype");
  font-style: normal;
  font-weight: 400;
  font-display: swap;
}

@font-face {
  font-family: "Outfit";
  src: url("./assets/fonts/outfit/static/Outfit-Regular.ttf") format("truetype");
  font-style: normal;
  font-weight: 400;
  font-display: swap;
}

@font-face {
  font-family: "Outfit";
  src: url("./assets/fonts/outfit/static/Outfit-SemiBold.ttf")
    format("truetype");
  font-style: normal;
  font-weight: 600;
  font-display: swap;
}

@font-face {
  font-family: "Outfit";
  src: url("./assets/fonts/outfit/static/Outfit-Bold.ttf") format("truetype");
  font-style: normal;
  font-weight: 700;
  font-display: swap;
}
    Marked as helpful
  • P
    Destiny•130
    @DetDet91
    What are you most proud of, and what would you do differently next time?

    I customized and used my social media instead of what the project provided. One of the small things I liked about my coding was making my social media open in another tab or window. I wanted to detail these minor things.

    Responsive Social Link page(with my actual socials)

    1
    P
    Rupali•890
    @rupali317

    Hi, @DetDet91 good work on this challenge. Good attempt at making the HTML semantic, especially the portion related to the list items. It is impressive that you have gone above and beyond to include the focus style for the buttons.

    I have the following feedback for your code:

    • In CSS, use custom properties for colors, typography, spacing. The advantage of using custom properties is that you have all these properties in one place and if you have to change any values, you only have to change the values of those properties, otherwise, you will have to change in all the other places, which is not maintainable. You can read about custom properties and then you can think about how to incorporate the suggested changes in this challenge
    • The attribution part should not appear within the card. It should appear after. So, please shift it after </article>
    • For font sizes, you should always use relative units (like rem, em) instead of fixed units (like pixel) since pixel is not good for accessibility. For example, if you alter the font size of the browser via settings to a larger size, the pixel will not scale. This significantly diminishes the user experience of those users who rely on reading larger texts.
    • While using <p> is fine for the text "Beginner front-end developer", you can also explore the <q> tag since it automatically adds the quote.
    • In the attributions, please use a different font color for the links since it fails the color contrast check and it is hard to read. Using a color with a better color contrast will improve the accessibility
    • I noticed a bug, where when I try to view your site on my mobile (landscape mode), the top part of the card is cut off. The reason is that in your body, you have a CSS property as height: 100vh. The fix: Modify it as min-height:100vh because the former is setting a fixed height for the body and hence it will chop anything if the content on your screen exceeds the viewport. min-height:100vh means that the body will have a minimum height of 100vh and hence it will let you scroll vertically. Additionally, consider providing padding on the body so that there is a breathing space around the card.
    • You have used a h2 for your location. In this case, a <p> tag is better suited for location. Headings usually introduce content. If you have h2 for location, the expectation is that the remaining content might talk more about the location. But, that's not the case. It is especially easier for users who rely on assistive technology to understand the content of your site. Using the wrong HTML tag can give a wrong impression.

    Hope the above suggestions help. If you need any clarifications, please let me know.

  • P
    Rupali•890
    @rupali317

    Hi @kimkim200949, to answer your question on the comment that you left for me at My blog preview component

    I suggest the following changes for the #img in your CSS:

    • Remove the fixed width and height.
    • Instead, add max-width: 100% since it is responsive. Ideally max-width: 100%; display:block; should be under your CSS resets. More on CSS resets
    • Remove the margin definition for the image since it adds extra space around the image thus giving an impression that the alignment is incorrect

    Final definition of the #img after the suggestions

    #img {
    max-width: 100%;
    display: block;
    border-radius: 2vh;
}

    Does this help?

  • P
    Rupali•890
    @rupali317

    Your solution is using rem for font-sizes, which is great for scaling. You have incorporated an animation on the hover state by having a duration of 0.2s. You have a good understanding of the flexbox

    I would like to suggest the following improvements:

    • To make the HTML more semantic, use <time> element instead of the <p> for the date. <p> element is more suitable for paragraphs, which normally spans across at least 2 lines. Similarly, the author's name can be represented as <span> instead of <p>
    • Another observation is that you have used <h3>. Normally, assistive technology would expect a <h1> tag to understand the flow of the document. What I would do is have an <h1 class="visually-hidden">Blogs</h1> and then wrap the HTML & CSS foundations under <h2> element. You need to write the CSS definition for visually-hidden class. You can refer to visually-hidden CSS definition Reasoning: The hidden <h1> will ofcourse be hidden for users but an assistive technology will read it out and will convey the structure of the page to users who rely on assistive technology, thus enhancing their user experience. The reason for using <h2> instead of <h3> is because in accessibility checks, one should avoid skipping heading levels.
    • In your CSS, you have defined the color definition under :root. You can define other properties like font sizes, line heights, spacings, border radius as well. You can refer to CSS Definitons under root
    • I tested your solution on my mobile and on the landscape mode, the layout breaks. The attribution appears in the middle. You should modify the CSS definition of the body element. You should change to min-height:100vh instead of height:100vh. Then you should apply a gap to have a space between the card and the attribution. Also, you do not have to use display:grid on the body. A simple display:flex should be enough instead since flexbox is appropriate for one dimensional view. Grids can be handy for 2-dimensional layout, which is not the case here.
    • In your CSS, avoid font-size: 62.5%; under html in your CSS. Refer to this article Should I change the default HTML font-size to 62.5%?
    Marked as helpful
  • DoomCrusher•120
    @VADER900000
    What are you most proud of, and what would you do differently next time?

    figuring out how to start my first project and using git and git hub and trying to figure things out instead of just using tutorials also centering using CSS and not just spamming

    What challenges did you encounter, and how did you overcome them?

    i encountered a challenge that the border radius (rounded corners) were not applying to the top of the QR code i figured out i must use margin and not padding also figuring out how to center was difficult but i used these align-items: center; justify-content: center; height: 100vh;

    What specific areas of your project would you like help with?

    more feedback on the semantic side as i don't know how to do that effectively also maybe tell me if any of my code could be done more effectively

    Qr Code

    #bem#accessibility
    2
    P
    Rupali•890
    @rupali317

    Hello @VADER900000

    Kudos on overcoming the challenges related to centering and applying the border-radius.

    I have the following suggestions for the HTML and CSS:

    • Your project should have a CSS reset otherwise different browsers will apply their default stylings. We want a consistent look and feel in all the browsers. Refer to this CSS reset article
    • For font size you should always use the rem unit instead of pixel. Pixel is not good for scalability and thus it will lead to poor accessibility. For instance, I am a user who relies on large font. So I will go to my browser settings to alter the font settings to a larger one. I will expect that the font in your qr code site to scale accordingly. But since you have used pixels, the font will remain the same and it will not scale to a larger one.
    • To improve reusability of the code, always define CSS variables. Please refer to the the CSS definition. Notice how the CSS variables are used throughout the CSS file. If we decide to alter a property value, instead of modifying it in various places, we can just alter the value from the CSS variables. The CSS variables are the ones defined in between :root{...}
    • In your HTML specify width and height attributes for the image element. Actually the .Qrcode class only needs the border-radius definition since width and height are specified in the HTML
    <img src="images/image-qr-code.png" class="Qrcode" alt="This is a Qrcode" height="288" width="288">
    • you do need the div with the class QrCodeMainContainer. Instead, you can have the styles like background-color, box-shadow etc styles in the qrCodeContainer class instead. Also, apply padding: 1.25rem; text-align: center; max-width:300px; to the the qrCodeContainer class. Let's aim for simplicity.
    • You have used <br> in your code. It is not needed. You can achieve the spaces using margin-top property instead.
    • Another tip to enhance the accessibilty of the project is to add the aria-describedby to the <img> element and have the corresponding id to the <p> element since the image and the paragraph have a connection. It will be easier for users who rely on assistive technology to form a connection between those two
    <img src="images/image-qr-code.png" class="Qrcode" alt="This is a Qrcode" width="288" height="288" aria-describedby="qr-code-instructions">
...
<p id="qr-code-instructions">....</p>
    • You have used `` in your project. While it is acceptable for this project since it is a small-scaled one, always keep your CSS in an external file as it promotes maintainability, reusability, better performance.
    • You have defined styles in .topParagraph class. I would instead define them in h1 since h1 has a lower specifity than .topParagraph class. Elements have lower specifity than classes. Lower specificity is better for reusability, maintainability, scalability.
    • Moreover, properties like width and margin are not needed in .topParagraph class since we are using max-width on QrCodeMainContainer class and applying width and height attributes to the <img> element. Similarly, the <p> element does not need width and margin CSS definitions.
    • Since we added a max-width to the card, we do not need the @media only screen and (max-width: 600px)

    Hope these tips help. If you need further clarifications, please ask

    Marked as helpful
  • P
    Srishti Chaudhary•250
    @itsmesrishti
    What are you most proud of, and what would you do differently next time?

    I'm happy about the banner images and how I used background properties to deal with them. Not sure if it was the best approach or I should have added them using html.

    I would like to write better css next time and have a proper arrangement for scss.

    What challenges did you encounter, and how did you overcome them?

    I was struggling with finding out the best approach to add the banner images. I thought maybe I should add them using or maybe image-set but I wasn't sure and I wasn't able to achieve what I wanted so I just went with adding them using url() with background-image and then using media query to replace the image.

    What specific areas of your project would you like help with?

    I would like help with this banner project thing what would have been the best apprach. Like should i have added them in html and then used media queries or maybe use or srcset or image-set?!

    Meet Landing Page

    #sass/scss
    2
    P
    Rupali•890
    @rupali317

    Hi @itsmesrishti

    I have reviewed your code, and in my opinion, the way you implemented the banner image is very well done. Given that the image is slightly cropped at the sides, using url() with background-image and background-position is an appropriate approach.

    I have the following suggestions for the CSS part:

    • I noticed that in a couple of places you have defined font-size in the html tag. Avoid using pixel since it does not scale and is bad for accessibility. rem is preferable due to the ability to scale.
    • In your variable file, avoid naming the variables like $purple-hover, $cyan-hover because what if the requirement decides to change the color theme from purple to red? That means the name "purple" is no longer valid and you will also have to change the name in various places. Choose a name which is more generic such as $primary-color, $secondary-color.
    • Besides defining color as CSS variables, you can also define other properties such as typography, border-radius, shadows, spacing. You can refer to this CSS example
    • Your project should have a CSS reset otherwise different browsers will apply their own default stylings. We want a consistent look and feel in all the browsers. Refer to this CSS reset article
    Marked as helpful
  • Ann•110
    @anastDev
    What are you most proud of, and what would you do differently next time?

    What I’m Most Proud Of and What I’d Do Differently Next Time

    • I’m really proud of sticking with this project and not giving up, even when faced with new challenges. It felt great to see my persistence pay off and to realize that I’m starting to retain more from previous projects—there were a few things I didn’t need to look up this time!

    • Next time, I’d like to explore alternative methods and approaches to see if I can make my workflow even smoother. I’m also interested in experimenting with different design elements and seeing how they can enhance the overall project.

    What challenges did you encounter, and how did you overcome them?

    For future projects, I want to:

    • Improve Responsive Design: Keep refining my ability to create layouts that work well on all devices.

    • Advance in CSS: Focus on mastering CSS animations, transitions, and grid layouts for more dynamic designs.

    • Enhance JavaScript Integration: Get better at combining JavaScript with CSS and HTML to add interactive elements and improve user experience.

    What specific areas of your project would you like help with?

    I’d appreciate any constructive criticism on how to improve my code, especially in making it more concise and well-structured.

    Responsive social links page - CSS Flexbox/Bootstrap/Hover-Focus State

    #bootstrap#accessibility
    2
    P
    Rupali•890
    @rupali317

    Hello @anastDev. Kudos for completing the challenge. Please refer to my suggestions:

    Does the solution include semantic HTML?

    The code can be more semantic as follows:

    • You should not use <h4> for the color coded phrase. Because the phrase is not a heading. <span> is enough. Headings are more appropriate if you are introducing a new concept (like how you see headings and subheadings used in a blog).
    • Similarly, <h5> is not correct for the quote. You can use <q> tag instead.
    • The button like structures are not a <p>. They are not paragraphs. They actually represent an element that will enable you to navigate to a different page. Therefore <a> tag is more appropriate.

    Is it accessible, and what improvements could be made?

    Once you make the above code changes, the accessibility of the site will improve. Currently, when I use TAB key to navigate your site, I was expecting to navigate the links as well (for which you currently have as <p>). Since you have them as <p> tag, I am unable to navigate those social links. Changing to <a href="#">Github<a> etc will help me navigate when I use TAB key on my keyboard. Keyboard users will appreciate that.

    Does the layout look good on a range of screen sizes?

    • Large screens -> There is huge space around the quotes
    • Mobile -> I tried in my mobile, I just noticed the right and the left edges of the card sticking to edges of the screen. The challenge requires some space between the edge of the right/left side of the card and the edges of the screen. I believe that is happening because of the width 50vh on the .card class. You should use max-width instead.

    Is the code well-structured, readable, and reusable?

    • In CSS, I would rename .card-img-top to profile-pic to improve the readability.
    • To improve reusability of the code, always define CSS variables. Please refer to the the CSS definition. Notice how the CSS variables are used throughout the CSS file. If we decide to alter a property value, instead of modifying it in various places, we can just alter the value from the CSS variables. The CSS variables are the ones defined inn between :root{...}

    Does the solution differ considerably from the design?

    • In larger screen, the reason there is a lot of space around the quote is because of .card-body class having flex:1 1 auto. You should remove it.
    • Also remove the fixed height of 80vh from the .card class. You do not need it.

    Another piece of advice: Your project should have a CSS reset otherwise different browsers will apply their own default stylings. We want a consistent look and feel in all the browsers. Refer to this CSS reset article

    Let me know if the above suggestions help!

    Marked as helpful
  • Jun Wei•200
    @junwei-wong
    What are you most proud of, and what would you do differently next time?
    • Using pure CSS, as I've been too reliant on component libraries
    What challenges did you encounter, and how did you overcome them?
    • Importing fonts, just read the docs or W3Schools
    What specific areas of your project would you like help with?
    • Anyway to do this in pure CSS without flexbox?

    Pure CSS with flexbox

    #pure-css
    2
    P
    Rupali•890
    @rupali317

    Hi @junwei-wong

    Congratulations on completing your first challenge. I have the following feedback for your code (in terms of enhancing the accessibility and semantics of the HTML):

    • You should always include a CSS reset for your projects. Different browsers may have different default stylings for UI elements. In order to make the CSS stylings consistent across all browsers , you should reset the CSS. Please refer to the following blog on CSS reset
    • For font size you should always use the rem unit instead of pixel. Pixel is not good for scalability and thus it will lead to poor accessibility. For instance, I am a user who relies on large font. So I will go to my browser settings to alter the font settings to a larger one. I will expect that the font in your qr code site to scale accordingly. But since you have used pixels, the font will remain the same and it will not scale to a larger one.
    • Your HTML structure in relation to the <main> tag is currently as follows:
    <main>
  <img>
  <div>
    <label></label>
    <label></label>
  </div>
</main>

    Semantic HTML is preferable since it enhances the accessibility of your page and it leads to better SEO. Your code can be more semantic as follows:

    <main>
  <img>
    <h1></h1>
    <p></p>
</main>

    A heading is represented as <h1>, <h2>, <h3>, <h4>, <h5>, <h6> A paragraph is represented as <p>. They are not appropriate as <label>

    • In your CSS, you have created CSS variables like --slate-300, --slate-500 etc. You can also extend using CSS variables for other properties like colors, font size, font weight etc so that you can reuse them and maintain the changes easily. For example, if the color scheme changes from blue to green, rather than having to change in various places, you just change the value of the CSS variable. For example:
    :root {

    /* Colors */
    --background-color: hsl(212, 45%, 89%);
    --card-color: hsl(0, 0%, 100%);
    --link-color: hsl(228, 45%, 44%);
    --main-text-color: hsl(218,44%,22%);
    --sub-text-color: hsl(220, 15%, 55%);

    /* Typography */
    --font-size-main: 1.375rem; /* 22px */
    --font-size-sub: 0.9375rem; /* 15px */
    --font-size-footer: 0.8125rem; /* 13px */
    --font-weight-bold: 700;
    --font-weight-regular: 400;
    --line-height-normal: 1.2;  /* Normal */

    /* Spacing */
    --space-none: 0;
    --space-base: 1rem; /* 16px */
    --space-m: 1.5rem; /* 24px */
    --space-l: 2.5rem; /* 40px */ 

    /* Border radius */
    --card-border-radius: 20px;  
    --img-border-radius: 10px;

    /* Box shadow */
    --card-box-shadow: 0px 25px 25px 0px rgba(0, 0, 0, 0.05);

}

    Let me know if the above suggestions help.

    Marked as helpful
  • P
    Smaylen5•600
    @Smailen5
    What are you most proud of, and what would you do differently next time?

    I’m proud of becoming more familiar with React, learning the basics of Formik, and using CSS variables effectively. I’m also pleased with having met some of the WCAG accessibility guidelines.

    What challenges did you encounter, and how did you overcome them?

    The most challenging part was figuring out how to position the logo whether to use a conditional render or the approach I ended up using. Since the component was already created, I chose to use Tailwind Merge to change classes rather than repeating myself.

    Another challenge was effectively using CSS variables with Tailwind, especially with gradients.

    What specific areas of your project would you like help with?

    I would appreciate feedback on:

    • Whether the code is semantically correct.
    • If there’s a better way to use CSS variables.

    base apparel whit tailwind, react, formik and yup

    #react#tailwind-css#accessibility
    1
    P
    Rupali•890
    @rupali317

    Hi @Smailen5

    Based on your question "whether the code is semantically correct", I would like to suggest the following:

    • Your code is missing a <h1> tag. Having 1 h1 tag is crucial for assistive technologies to understand how the page is structured. In your case, instead of h2 you should use the h1 tag and style it accordingly.
    • You have used <section> tag to enclose the header and the content. The logo can be enclosed in a <header> tag instead of a generic <section> tag. The content can be wrapped in a <main>. <main> and <header> are examples of HTML landmarks. These tags are more semantic than a <section>.
    • You can consider using <input> tag's pattern attribute to handle the regex check. It is better to put the responsibility to the CSS since doing the regex code on CSS will mean less code on the Javascript side. There will be performance improvement. Browsers are optimized to handle CSS operations more efficiently than JavaScript.

    Let me know if the above suggestions help!

    Marked as helpful
  • P
    MAGENE Sem Joel•320
    @Jomagene
    What are you most proud of, and what would you do differently next time?

    I'm most proud of integrating custom fonts using @font-face and maintaining a responsive layout through a mobile-first approach. Next time, I’d explore CSS Grid for more complex layouts and focus more on performance optimization, like using font-display: swap and serving fonts in WOFF2 format for faster load times.

    What challenges did you encounter, and how did you overcome them?

    A significant challenge was creating a responsive design that adapts seamlessly across different screen sizes without overusing media querries. I used Flexbox for layout management, which allowed me to center elements both horizontally and vertically with the my custom classes .flex and .flex-center. Additionally, I employed a media query to adjust the width of the list of links on larger screens, ensuring a consistent and balanced layout.

    What specific areas of your project would you like help with?

    Any suggestions for enhancing the code organisation and accessibility, would be appreciated.

    Responsive Social Links Profile with Custom Fonts & Flexbox

    #accessibility#pure-css
    3
    P
    Rupali•890
    @rupali317

    Hello @Jomagene

    Great work on this challenge. I have the following feedback:

    • In terms of accessibility, you should never use pixels for font size because if I rely on large fonts and I go to my browser settings to alter the fonts to larger ones and then I come to your project, I will see that your fonts have not scaled. It is not great for accessibility. That has happened because you use pixels. Pixels are not good for scaling. Pixels are fixed. You should use rem instead.

    • For the quotes, you can use the <q> tag

    • I noticed that the right side of the image seems to be chopped off. Unfortunately, I am unable to upload the screenshot. This can be fixed by eliminating the <section class="image-container"></section>. It is not needed. Just the <img> related code is enough and in your CSS of the img, you apply the border-radius. You already have the height property for image. Try to keep the code simple. No need to complicate the code.

    • Another piece of advice: Your project should have a CSS reset otherwise different browsers will apply their own default stylings. We want a consistent look and feel in all the browsers. Refer to this CSS reset article

    Let me know if the above suggestions help

    Marked as helpful
  • P
    Daniel Basilio•420
    @danielbasilio
    What are you most proud of, and what would you do differently next time?

    I'm very proud of the responsive layout made using CSS grid. In the future I would focus on improving the separation of components within sass

    What challenges did you encounter, and how did you overcome them?

    The project as a whole was quite challenging. It took me a long time to finish, as I am a beginner in React. I had to study the framework throughout all stages of development.

    What specific areas of your project would you like help with?

    I believe that the entire architecture can be improved.

    Newsletter Sign Up using sass and css flexbox

    #accessibility#itcss
    1
    P
    Rupali•890
    @rupali317

    Hello @danielbasilio

    Front-end mentor team requested me to review your code and provide feedback since I am following the Javascript learning path and I have also finished this particular challenge. It is my pleasure reviewing your code and offering suggestions for the code improvement:

    Your HTML code is semantic (especially how you used the address tag and the picture tag).

    My suggestions:

    • In the browser, when I altered the font settings to large, I noticed that some of the font sizes did not scale accordingly. This is because you used px as font size. I suggest you use rem unit to make the fonts scalable otherwise the text will not be accessible to those who rely on bigger font size via browser.

    • You used an inline style of display:block and display:none. You can eliminate the usage of this inline style and instead toggle the appearance of hide-error class. element.classList.toggle("hide-error") will achieve it. Classes have lower specificity than inline styles.

    • The javascript functions like displayError and enableButton can be renamed as toggleInlineError and toggleButtonState respectively. The former names did not convey the actual functionality and hence it becomes difficult to understand the code at an initial glance.

    • When I typed the correct email, I expected the disabled state of the button to go away. But it did not do so. It is a bug. In your "displayError" function, you also need to handle the state of the button. So based on that, please rename the function accordingly so that the purpose of the function is conveyed.

    • The variables in the javascript are well named. One point where you can improve the performance is not to always use document.querySelector(). I want to highlight the document part since it will check from the entire document. It is doing so 6 times since you declared 6 variables. You can improve it in this way:

    const $formSignUp = document.querySelector('[data-js="form-sign-up"]');
const $emailField = $formSignUp.querySelector('[data-js="form-email-field"]');
const $btnSubmit = $formSignUp.querySelector('[data-js="form-btn-submit"]');
const $modal = document.querySelector('[data-js="modal"]');
const $btnModal = $modal.querySelector('[data-js="modal-button"]');
const $errorMessage = $formSignUp.querySelector('[data-js="error-message"]')

    In the above example when I want to select an email field, I do not have to use document again since I have already searched sign up form. To select the email input, I can search it from the sign up form rather than combing the document again.

    • The modal component's email should be based on what the user has typed on the email input field. Currently it is ash@loremcompany.com. Also, it is nice to see that you have made the email address as a link. However, clicking on that email is showing a 404. I think you wanted to open an email. To do so, you need change it as: <a href="mailto:ash@loremcompany.com" title="Confirm your email" class="link">ash@loremcompany.com</a>
  • Thiago Amâncio•50
    @MoonAmon
    What are you most proud of, and what would you do differently next time?

    I enjoyed working on the composition with JavaScript and modeling the popup. However, I had a lot of difficulty with CSS positioning. I tried my best and will review where I went wrong.

    What challenges did you encounter, and how did you overcome them?

    The main challenges I faced were positioning the popup and implementing it with JavaScript.

    What specific areas of your project would you like help with?

    I need help with the DOM manipulation part of JavaScript. I believe the logic is not functioning correctly.

    Responsive Card with Popup Javascript

    1
    P
    Rupali•890
    @rupali317

    Hello @MoonAmon

    My response for your comment " need help with the DOM manipulation part of JavaScript. I believe the logic is not functioning correctly." -> I went through your Javascript code and you actually do not need 90% of the logic. The only place where Javascript is needed is when you click on the share button to show/hide the tooltip. That's all.

    My advice is that before writing the Javascript, spend some time to plan what you need to build.

    1. When user clicks on share button, the tooltip will show
    2. Next time, when user clicks on share button, the tooltip will hide
    3. and so on...

    That means you need the following logic (i.e, how you will build the above steps):

    1. You need to select the share button
    2. You need to select the tooltip element so that you can hide/show it
    3. After selecting the share button, you need to add an event listener to it (The click event)
    4. Inside the callback function, you write the logic for hiding/showing the tooltip.

    Image your HTML is as follows:

    <div class="tooltip">
// Rest of the tooltip content goes here
</div>
<button>Share</button>

    Your CSS has the following class. This will be handy in hiding/showing the tooltip

    .invisible {
 visibility:hidden;
}

    A sample JS logic

    // 1. You need to select the share button
const shareButton = document.querySelector("button");

// 2. You need to select the tooltip element so that you can hide/show it
const tooltip = document.querySelector(".tooltip"); 

// 3. After selecting the share button, you need to add an event listener to it (The click event)
shareButton.addEventListener("click",()=>{
  // 4. Inside the callback function, you write the logic for hiding/showing the tooltip.
  tooltip.classList.toggle("invisible");   // The invisible class that is defined in the CSS
})

    You can refer to my solution

    You can refer to the following Javascript resources. It is the best resource so far in my 10 years of working with Javascript:

    Hope this helps!

    Marked as helpful
  • P
    Rupali•890
    @rupali317

    Hello Eric

    I am following the learning path for Javascript. While following this learning path, the front-end mentor team has requested me to provide feedback on your solution. I am delighted to share the following suggestions for your approach on article preview component:

    A) HTML Semantics

    • You should use h1 instead of h3. Especially for assistive technology, they rely on these headings to understand the page structure. h1 will tell the assistive technology that there is a main heading and it will help it understand how the page is structured.

    • Instead of <div class="left"></div> how about you use the <img> tag? It makes the HTML more semantical. Semantical structure is better for assistive technology.

    • For the share icon, you can consider having a <button> tag wrapping it because the share is supposed to be an interactive element that toggles the tooltip.

    B) CSS improvements

    • The .share__box has a fixed width and that is why the tooltip is not showing properly in the mobile view. You might want to adjust the dimensions accordingly to the mobile view.

    C) General comments

    • Use rem instead of px as the former is ideal for scalability.

    • You are having the script towards the end of the body. Another approach in terms of better performance, is to have the script within the head tag and add defer attribute: ``. This means that the script and the HTML will be loaded in parallel and only when the entire DOM is loaded, the script will then be executed. In your approach, the script downloads and executes only after the loading of the DOM, so it is slower.

    Hope the above helps. Let me know if the suggestions work

    Marked as helpful
  • P
    Hexerse•460
    @Hexerse
    What are you most proud of, and what would you do differently next time?

    I am proud of being able to use grid properly and being able to align the names as well as adding the background image. I will align the names properly next time, so I do not have to use grid to do it for me.

    What challenges did you encounter, and how did you overcome them?

    I am having trouble with the content I realised my content was too large. I did not overcvome it as it would take so much more time. But I realised why it happened.

    What specific areas of your project would you like help with?

    are my objects too large ?

    testimonials-grid

    1
    P
    Rupali•890
    @rupali317

    Hello @Hexerse

    Congratulations on completing this challenge. The grids are looking great. As for your question on if the objects are too large, it is up to you if you want them to be large. If you want your solution and design to match then you can adjust the dimensions accordingly.

    Also, I viewed your site in my mobile and some part of the purple card is being chopped off. You might want to take a look at it. I believe it is due to height: 100vh in the <body> tag

    Marked as helpful
  • timdogan0•30
    @timdogan0
    What are you most proud of, and what would you do differently next time?

    I'm proud of the fact that I am getting faster at doing these challenges

    What challenges did you encounter, and how did you overcome them?

    I struggled with centering stuff.. again 😂

    What specific areas of your project would you like help with?

    I would like to know if I am doing things in a way that is using best practices.

    Social Profile Links Card using HTML/CSS

    2
    P
    Rupali•890
    @rupali317

    Hello @timdogan0

    I am in agreement with @danielmrz-dev's suggestions.

    Additionally, I have a few pointers that can enhance the structure and accessibility of your code:

    1. Your project is missing a <h1>. Instead of <h2> you should use <h1>. Headings facilitate page navigation for users of many assistive technologies. They also provide semantic and visual meaning and structure to the document. A first level heading <h1> should be present on nearly all pages. It should contain the most important heading on the page (generally the document title).

    2. For the "Front-end developer and avid reader", instead of <p> you can use the quote tag <q> as it is more appropriate as a quote.

    I suggest you use WAVE, Accessibility insights to test how your project is doing in terms of accessibility.

    Let me know if this helps.

  • Joseph K. L Lartey•430
    @FlackoCodes
    What are you most proud of, and what would you do differently next time?

    Getting my hands dirty with useSate hook in React for updating the errors.

    What specific areas of your project would you like help with?

    Got stuck with updating the form inputs color to red when the are erros.

    Responsive age calculator app

    #accessibility#react#pure-css
    2
    P
    Rupali•890
    @rupali317

    Hello @FlackoCodes

    For updating the inputs to red color border, you need to inject a class to it like .error-input when the error occurs. When the input is fine, then the .error-input class should be removed. Then in your css apply the respective css property to that .error-input class.

    I have also worked on a similar project where I need to validate my inputs and display the errors accordingly for the inputs. I used React and Styled components for the project. Refer to my Input.jsx.

    Hope it helps you in solving your issue.

    Marked as helpful