Skip to content
OverviewSolutions
21
Comments
47
P

Rupali

@rupali317Singapore890 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • 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
View more comments