Skip to content
OverviewSolutions
2
Comments
9
Daniele Manca
@danielemanca1983

All comments

  • Daniele Manca•160
    @danielemanca1983Posted over 1 year ago

    Hi Shreyansh,

    I have taken a look at your solution and it is really good, the only thing I would suggest to invest some more time into is the positioning of each element and the actual font sizes.

    As a front-end developer it is a must to be able to develop pixel perfect solutions in almost every company.

    Other than that great work.

    1
  • Daniele Manca•160
    @danielemanca1983Posted over 1 year ago

    Hi Maneesh,

    I am here providing you some constructive feedback in regards to your submitted solution.

    First of all I would like to say that your solution is pretty good, however here are a few suggestions which would enable you to further improve the solution:

    1. your .top-left-corner and .bottom-right-corner divs should be slightly readjusted, if you look at the actual design, their position does not match exactly, on your solution. When you will be applying for job and be assigned with a technical challenge, you will greatly risk not to get the job if you have mismatches between given UI designs and your solution.

    2. Inside the .stats div, you could make use of BEM CSS classes, for instance you have nested divs inside of .stats, which have no class, you can look into BEM here, although it's not a must to use it, in my opinion it's important to know what it is and why it's useful.

    Other than that it's pretty good, :)

    Marked as helpful
    0
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hi Musa, hope you're doing well.

    Please allow me to provide you with some constructive feedback, your solution is pretty good, however I believe in terms of mark-up, it can be improved a tad.

    • The challenge, based on the design, does not absolutely require an H1, however I'd suggest to perhaps use one on the Notifications text, because frontendmentor does require to have an H1 on each submitted challenge, this is also to get folks used to apply as much semantic mark-up as possible and get developers used to implement accessibility into their work.

    • Same goes for the images, all images on the web require a descriptive alt attribute to enable screen readers access the image description for people with certain visual impairments, including blindness on different levels.

    The CSS and Jquery code are pretty good, well done overall.

    Marked as helpful
    1
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hi Gaurav,

    Nice solution to the coding challenge, however I'd like to provide you with a suggestion for future reference, to make the code even better.

    For instance you have an .image-container class in your mark-up, which you target in the CSS and assign an image to through the background-image property, in this case, I think the best would be to have a picture tag in the mark-up and use the srcset attribute where you can have multiple instances of the image, which makes it also accessible to screen readers, as using background-image is not accessible at all by screen-readers.

    Here is an example of how to use the picture tag in HTML: picture

    Other than that it's a really good solution, :)

    1
  • a-woodworth•290
    @a-woodworthSubmitted almost 2 years ago

    Hi everyone! 👋

    Finally getting around to posting my solution for this challenge. Tried to make this tip calculator accessible, but still learning a11y so did my best.

    The math shown in the Figma example had me switch out how I calculated my total split tip. Had initially rounded up all the values.

    I'm sure there are areas in my code that could be improved, but the calculator is working so time to move on to another challenge.

    Happy Coding!

    Tip Calculator App - Built with CSS Grid, Flexbox & JavaScript

    1
    Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hey there,

    I think your solution to this challenge is really good, mark-up is well written and using semantic tags, the CSS modular architecture you have followed is also very good, the UI for the component closely resembles the design provided in the challenge, and the JavaScript functionality has also been written very well, with all the relevant variable declarations and their utilisation in the relevant functions.

    Really a great solution, keep up the great work,

    :)

    Marked as helpful
    0
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hi Sakyun,

    Hope you're doing well,

    Great solution to this challenge, I would just like to provide you with some constructive feedback so that you can perfect the solution even more.

    • In modern client-side development, it's best practice to wrap the content inside of a <main> tag

    • You have both a .container and a .grid classes in your mark-up, only one is necessary, you can apply the max-width and margin properties to center the container and also apply the grid properties to make the container a grid container all in one class.

    As for the rest I believe you did very well, good CSS variables set-up in your stylesheet under the :root declaration and great translation of the design into code, well done.

    Marked as helpful
    0
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hey there,

    I will be providing you some guidance on how to improve the solution you submitted, however I would suggest that, not only you look into flexbox in terms of positioning elements on a page, but also review HTML, specifically headings 1 to 6, as headings should not be skipped, let's say for instance as on your solution, from 1 all the way down to 6.

    I will be touching on the mark-up code first and then move onto the CSS.

    HTML

    • Your code contains two containing <div>s, one only is sufficient, and I would suggest to take a look at the <main> tag as well, which in modern web development we can wrap content into, see its usage on this MDN Link

    • So to recap briefly from the above, you could have: main followed by the card component, no other containers.

    • As already mentioned by someone else the <h1> does not need to be split in 3, one tag is sufficient and the text will wrap by itself

    • Also, as someone already mentioned, in order to position the discounted price and the original price, you could look into flexbox, for instance you can have a heading <h2> in there with a <span> for the smaller text, display the heading as flex and then align-items: center to vertically center the smaller text with the larger one.

    • The mark-up code for the <button> element is good.

    Now the CSS

    • I think that, again in terms of flexbox, you can position the component vertically and horizontally centered on the page, by using the flexbox properties or also grid, they both do a great job at doing this.

    • Using the float property requires the element coming right after to be cleared, with the clear property. However if you used flexbox or grid that would not be necessary at all.

    • I would suggest not to assign a fixed width and height to the img, it kills it from being responsive at all, instead give it a max-width.

    • The .btn CSS rule contains properties which are not required based on the given design for the component. For instance the <button> in the design only makes use of a bold green background color, no border, for which you can use the border: none; CSS property, and then apply all other properties to the button to style it as per the provided design.

    • The component on the mobile design displays the content in a vertical fashion, one potential way of achieving that, is to use flexbox on the component to make its children flex items and then use the flex-direction property for smaller viewports to display the content as a column.

    Marked as helpful
    1
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hi Mohsin,

    I am here providing you with as much constructive feedback as I can based on the code you wrote for your solution, it is quite good to be honest, but it takes another set of eyes, to notice any imperfections we can improve on.

    I looked at your solution and the dimensions of the component do not closely match the ones from the given design, for instance you should assign the .card component a max-width for larger screens. By adjusting the max-width, the height will auto adjust itself, as it is also off course currently.

    In terms of the HTML code you wrote you did pretty well, although I suggest that your .img-wrapper element be switched to a <picture> instead and also I would suggest not to hardcode the height in the <img /> inside of it.

    Your CSS code is also good, however I notice that there are some CSS declarations with no code within them, if you do not need them at all, I would suggest to remove those and re commit the code without them.

    In general you did pretty good, well done.

    Marked as helpful
    1
  • Daniele Manca•160
    @danielemanca1983Posted almost 2 years ago

    Hi Yusuf, Here is some feedback:

    1. Your solution's mark-up code could greatly improve by using semantic mark-up tags, as someone else already mentioned, you could wrap the content in a <main> tag. It is preferable to use semantic html tags instead of aria roles.

    2. I would have enclosed the actual component inside a <article> tag instead, by perhaps assigning a class to it such as card and then all the inner children elements such as the image and text could use BEM methodology for their class names. You can take a look at what BEM is on this link about [BEM] (https://getbem.com/introduction)

    3. The image could be enclosed inside a <picture> tag for semantics.

    4. Typography: the font-family in your solution does not seem to match the ones in the original design.

    5. For the button you have actually used a <p> tag, you must used either an anchor or a <button> for that purpose as the element on the design is a call to action. It also does not have a drop-shadow as per the design.

    6. Cancel the order should also be using an anchor tag instead, as it also is a call to action. In general we would use a <p> tag only for regular text elements.

    7. I would suggest the use of classes instead of IDs, unless we need an ID to bind the html element to a piece of javascript code in order to add some interactive functionality to it.

    It may seem like a lot, but believe me, as I am a front-end developer, I would get this sort of feedback whenever demoing a completed project to the visual designers, if there were mismatching elements.

    Marked as helpful
    0