Skip to content
OverviewSolutions
28
Comments
12
P
Matt Pahuta
@MattPahuta

All comments

  • Matt Pahuta•580
    @MattPahutaPosted about 1 month ago

    Nicely done completing this challenge. Your solution is a close match to the design comp, your JS code is nicely concise and easy to understand, and you're using a lot of great responsive units and custom variables in your CSS. I especially like how you've used the clamp function to handle the font sizes and keep the styles simple. However, I did go ahead and flag a few areas where you might consider improving your solution:

    • Media queries should be defined using rem or em units. Also, I think you could probably get away with just a single media query for this particular project.
    • On larger screens, your app is stretching awkwardly wide. Rather than setting a width property in percentages at different breakpoints, a better practice is to use a max-width using rem units.
    • Your app functions well, but for me, this is one big form, especially since we have input elements involved. A future revision would be to organize the html within a form element and allow for some of the native form behavior to assist in collecting the data and more gracefully rendering and announcing errors.
    • You might consider adding the 'step' attribute to your bill input field so that the spin buttons can increment/decrement the amount accurately. As it is now, you can only go up and down by whole numbers.
    • I'm confused about the label element you have for the 'select tip' section. labels should be paired with an input but this one is standing alone. It will return an error using an online validation tool like this one. This section is a good use case for a fieldset element with a legend instead of a label. The buttons could be radio inputs styled as buttons.
    • After entering values and calculating the amounts, using the reset button triggers the 'number of people' error to render. You should be able to fix this with some simple logic added to your JavaScript.

    Again, well done completing the challenge!

    Marked as helpful
    1
  • Matt Pahuta•580
    @MattPahutaPosted about 2 months ago

    Excellent work with this challenge. Your result is very close to the design comp, you've incorporated a lot of good semantic elements, your styles include a proper CSS reset, and are well commented and organized, among other great aspects of your project. I did go ahead and flag a few things you might consider for revisions:

    1. While you may have multiple header elements on any given page, if there's only one it should generally sit outside of the main element. The structure for this challenge is probably best suited for header-main-footer.
    2. Image alt descriptions should not include words like 'image' or 'picture' because they are already an image role. There are some great articles written on the subject over on the discord server, in the resources channel.
    3. It would be more semantically correct to use button elements for the design's buttons rather than styled a tags.
    4. The footer section content doesn't necessarily have any less weight than what came before. I think the heading here should be an h2 also.
    5. I recommend to avoid using the 62.5% font size hack. This is an outdated method of handling font sizing and can create a lot of accessibility issues for impaired users. Here's a quick explainer on the topic where Grace makes the case against using it. I tend to agree with her reasoning.
    6. The spacing between the accent text above the second heading looks a bit too tight.
    7. The last heading is stretching a little too long on larger screens.

    Again, very nicely done. Good luck moving forward!

    0
  • Matt Pahuta•580
    @MattPahutaPosted 2 months ago

    Hey there. Nicely done completing this challenge. Your solution is a close match to the design comp and you're effectively using semantic html elements as well as CSS custom properties. That said, there are a few things I'd recommend revisiting to improve your project:

    1. Get in the habit of including a modern CSS reset at the start of the styles. Andy Bell and Josh Comeau both have excellent resets you can google and use.
    2. There's no need to set a height on the body. This should be avoided. Instead, set a min-height of 100vh or 100svh on the body.
    3. The person's name should be a heading for each card instead of a p element. And as this is a component that's meant to live among other components on a page, you should make those h2's.
    4. Your styles and media queries are set up for larger screens first, then adjusted for smaller devices. This should be reversed. Build the layout from a mobile-first perspective and then use media queries with a min-width (and rem units) to account for the larger screens. You'll end up with cleaner css code and less you'll need to apply within the media queries. For instance, your grid container only needs a display: grid and gap by default. You'd then adjust the grid layout to your liking as the screen expands.
    5. Use rem units for font sizes instead of pixels. Also, It would be preferable to apply the font size on the body rather than the root. You could still define a variable for font size in root and use that as the value you assign to the body.
    6. Avoid using percentages with the width property. You have this set in several areas of your code and it's likely been a headache to get everything positioned properly. When dealing with widths, it's best to stick with max-width and/or min-width along with responsive units (rems or ems). The same goes for height properties.
    7. On the largest screen sizes, your design is stretching much wider than it ought to. Again, you'll want to adjust how you're setting your widths on the grid container and in other areas to fix this.
    8. The quotation image is purely decorative and should have empty alt attributes (alt=""). Also, the alt attributes for the user avatars should be a bit more descriptive. There are some good posts on writing good alt descriptions over on the discord sever in the resources channel.
    9. Back to the quotation image. I see you're using opacity to allow for the text to be visible but it's still partially obscuring the text. Use z-index to move it back in the stack or you could try applying the image as a background image and play around with the background positioning.

    Again, well done completing the challenge, and good luck moving forward.

    Marked as helpful
    0
  • Matt Pahuta•580
    @MattPahutaPosted 2 months ago

    Hi there. Great job on completing this challenge! You've matched the design comp quite well and achieved the layout using CSS Grid, which is the technique this challenge is designed to highlight. I also appreciate you correctly identified the icon images as being purely decorative and gave them appropriate alt values (empty values).

    As a previous reviewer mentioned, headings should always go in order to communicate the structure of the content correctly to assistive technology and bots. This design calls for one h1 tag and four h2 tags.

    Here are a few other things I flagged for you:

    1. Back to the main page heading, this should be one h1 split over two lines. You can wrap half in a strong tag or styled span with display set to block.
    2. You're using a lot of IDs in your html where you should be using classes. It's generally best practice to style elements using classes rather than IDs or by element tag. IDs are better suited for targeting elements via JavaScript, page navigation, linking labels with inputs, and a few other use cases. There is no need for IDs in this particular challenge.
    3. The cards don't need an extra wrapping div. One div within your main tag is plenty. You should always strive to keep your html as simple as possible.
    4. Get in the habit of using a modern CSS reset for all your projects. Andy Bell and Josh Comeau both have excellent resets you can google and use. Place it at the beginning of your styles file.
    5. Font sizes should always be set using rem units, not pixels. You're using a mix of both here, so you'll want to be consistent throughout your code.
    6. Media queries should also be defined using rem or em units, not pixels.

    Again, nice work here and good luck moving forward!

    0
  • Aaron Smith•280
    @medic-codeSubmitted 5 months ago
    What are you most proud of, and what would you do differently next time?

    Getting the JS to work properly. Not sure there is much I'd change differently, process to creating this was actually relatively smooth. I'm not sure I did the overlapping cards particularly well as i had to constrain them quite abit it felt like there was probably a smarter way.

    There's a few parts of the design like icon placement i'd individually place using position absolute maybe and few parts where alignment is a little off.

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

    Not too many challenges, mostly smaller ones around overlapping cards and overflow.

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

    HTML semantics CSS - Structure, refactoring potential, feedback on the overlapping cards JS - syntax and approach to the problem.

    Responsive Dashboard

    2
    Matt Pahuta•580
    @MattPahutaPosted 2 months ago

    Hey there. Great job on this challenge. You've matched the design comp really well and have incorporated some good semantic elements and modern, responsive styles. Your JS code is straightforward and efficient. You also interpreted the ui pattern here correctly as a tab interface. So good on that. I completely failed to recognize that myself when I worked on this project.

    I remember having trouble getting the overlapping cards to behave as well and just ended up using various padding values, not unlike your solution. I'm sure there's a more elegant way to achieve the effect but It's beyond me at this point.

    All that said, I did flag a few small things for you:

    1. The alt description on the user's avatar should be more descriptive and shouldn't include words like 'image' or 'picture' because they are already an image role.
    2. I believe the card icons here are decorative so the alt attributes can be empty.
    3. You're skipping some heading levels, moving from an h2 to an h4 and back to h2.
    4. The ellipse images are meant to be buttons. I'm guessing in a real-world app they'd open up a modal or another menu of some kind. It's not a detail I incorporated in my solution either but it's on the list for a future revision item.
    5. You still have the hard-coded data in your index.html file, so you should be able to delete that since you're populating values via JavaScript.
    6. For the JS, your 'work' variable isn't being used for anything I can see and can be deleted.

    Again, great work on the challenge. Cheers!

    0
  • Matt Pahuta•580
    @MattPahutaPosted 2 months ago

    Hey there. Nicely done completing this challenges. Other than a few finer details, your solution is a close match to the design comp. Good use of semantic elements and responsive units for your font sizes as well. Here are a few things I've flagged for you:

    1. On mobile, the spacing between the image and the heading is a bit off. I think you can remove the margin top from the h1 to remedy that.
    2. You're missing a border radius on the accent box (prep time).
    3. Also, there should be a similar border radius applied to the card and the image on larger screens.
    4. The height and width you have set on the image have no effect and are not necessary.
    5. Also for the image, the alt attribute should be more descriptive. Image descriptions should not include words like 'image' or 'picture' because they are already an image role. There are good posts in the discord server in resources channel on this topic.
    6. The bolded words in the lists are an appropriate use case for the strong tag. The spans you're using have no semantic value. Switch them out for strong tags and save yourself some code in the styles.
    7. You're using a table element for the tabular data so that's great, but your missing header cells. The 'Calories', 'Carbs', 'Protein', and 'Fat' cells should be th elements with the scope attribute set to "row"

    Again, great job finishing the challenge! Keep going. Cheers!

    Marked as helpful
    1
  • eros77sc•160
    @eros77scSubmitted 3 months ago
    What are you most proud of, and what would you do differently next time?

    Pleased to have developed a color palette with contrast ratios according to WCAG.

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

    I found it challenging to determine the sizes of the elements (and I’m still not satisfied) because I haven’t yet developed an eye for understanding measurements. I tried using a ruler and converting centimeters to pixels to get an idea, but in the end, I adjusted things visually.

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

    I welcome all tips, especially on how to make an element behave the same way regardless of screen size (for example, laptops and desktops). Is this defined in the body?

    Social Links Profile

    1
    Matt Pahuta•580
    @MattPahutaPosted 3 months ago

    Hi there. Nicely done completing this challenge. Your markup and styles are concise and efficient and use correct semantic elements. Here are some other things I have flagged for you:

    1. Use a modern CSS reset at the start of the styles for all your projects. Andy Bell and Josh Comeau both have excellent resets you can google and use.
    2. Other than height: 100% the properties you have set on html, body should only be on the body. Set a min-height: 100svh or min-height: 100vh on the body along with the other props. Some of this would be handled for you when you include a reset in your code.
    3. You don't need to set a width and height on your main element (and you really shouldn't). For this challenge, you might achieve better results by consolidating your flexbox properties on just the body instead of on both body and main.
    4. There's no need for a max-height on the card. In fact, max-heights are rarely used anywhere.
    5. The box-sizing property should typically be set globally instead of on an element farther down the tree (you have it set on the card div). This is also handled for you with a proper CSS reset.
    6. If you're going to use a width on this card, it should take a CSS function like clamp(), like you're using for the default font-size, otherwise it should be a max-width. Using clamp would achieve the varied dimensions of the card across screen sizes that you've asked about.
    7. The padding applied to the card looks a little too high compared to the design comp.
    8. For images that are meant to have an exact size (like the profile img here), use width and height. You can set it in either pixels or rem units (if you need it to scale). In this case, I believe the image dimensions are the same across viewport sizes so pixels is fine.
    9. This component features a list of social links. Use an html list to contain the links.
    10. The hover state bg/color changes on the links should apply when navigating via the keyboard as well. Add a focus-within pseudo-class to accomplish this.
    11. Nice work going the extra mile to try and improve the contrast ratios. However, it looks like your final results actually have lower ratios than the design comp. You can quickly see this in Chrome using the dev tools and selecting the elements. Here's another tool you might find useful.

    Again, great job finishing this project! Keep going. Cheers!

    Marked as helpful
    0
  • Spencer•80
    @DivinitrySubmitted 4 months ago
    What are you most proud of, and what would you do differently next time?

    proud of : turned out how I wanted it to with no issues

    need support on: naming my html classes I am not good at it and would like to improve for clarity

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

    issues : trouble getting the on hover transition to work but figured it out eventually

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

    the comment on classes, help with that would be great, best naming conventions?

    Blog preview card completed

    4
    Matt Pahuta•580
    @MattPahutaPosted 4 months ago

    Hi there. Nicely done completing this challenge. Your finished product is a close match to the design comp and you're using a lot of semantic element tags properly. Also good work with your use of custom properties and responsive units.

    Regarding the naming of CSS classes, this is often a challenge for devs. For some good insight on the topic, check out this video from Kevin Powell.

    Here are few other things I flagged:

    1. You should always have a main element on the page. Nest the div with the class name of 'main-container' within a main tag. Then, for clarity, you might consider renaming the 'main-container' class to 'card'.
    2. Since this is a card component that would sit along similar components on a page, it would likely never serve as the page title. Semantically, the heading should be an h2 instead of an h1 (even though the formatter will give you a warning about it).
    3. Your alt text for the image should just be 'Greg Hooper' or even something like 'Greg Hooper's avatar'. Image descriptions should not include words like 'image' or 'picture' because they are already an image role.
    4. You should have at least one fallback font where you have your font-family set.
    5. Remove that width from your card component ('main-container'). Give the body some padding instead and stick with max-width only on the card (but probably update that to use rem units instead of pixels).
    6. I think it's generally preferred to use unitless values for line-heights, but that might be debatable.
    7. You should avoid using percentages for margin and padding values. Stick with rems, ems, or even pixels.

    Again, good work on this project. Good luck moving forward. Cheers!

    Marked as helpful
    1
  • BertSki90•30
    @BertSki90Submitted 4 months ago
    What are you most proud of, and what would you do differently next time?

    I am proud to be able to centre the card horizontally and vertically on the screen. I would like to know different solutions on how to center the card horizontally and vertically on the screen.

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

    Centring the card vertically on the screen was the more challenging part. I found articles on freeCodeCamp and W3 schools.

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

    I want to learn more ways to centre a card on the screen.

    Mobile-first design using CSS flex

    1
    Matt Pahuta•580
    @MattPahutaPosted 4 months ago

    Hi there. Nicely done completing this challenge. Your finished product is a close match to the design comp and you've included helpful custom properties. However, I see some issues that should be addressed for this project as well as some things to keep in mind for the future:

    1. You should always have a main element on the page, so good on that, but it should not be used here for the card component. Instead, nest a div (or possibly a section tag) within your main element as the card component.
    2. To answer your question about centering the card, you're actually already using a preferred method with your flexbox properties on the body. You can achieve the same result with CSS Grid but for this project, I think flexbox is the better way to go. For a deep dive into centering elements, check out this article on Josh Comeau's blog
    3. Get in the habit of including a modern CSS reset at the start of the styles. Andy Bell and Josh Comeau both have excellent resets you can google and use moving forward.
    4. Use min-height on the body instead of height. There are very few occasions when you'll set a predefined height on an element and this should definitely be avoided on the body.
    5. On the card itself (again, this should be a div, not the main tag) you don't need to use flexbox. With no orientation switch between mobile and desktop that needs managing and with just three elements within the card, margins and padding are all you need. The block elements within the card will stack vertically by default.
    6. You should use max-width for the card component, not width. Also, set this in rem units not pixels.
    7. The image shouldn't have a specific height or width. Instead, use max-width: 100% on the image.
    8. You have an invalid property on the h1 tag (weight). You're looking to use font-weight here.
    9. For better accessibility, font sizes should always be set using rem units not px.

    All in all, very nice work here and congratulations on completing the project. Good luck moving forward. Cheers!

    Marked as helpful
    1
  • Martin Mwaka•390
    @TemceoSubmitted 6 months ago
    What are you most proud of, and what would you do differently next time?

    Being able to combine the use of html, css and javascript.

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

    My main challenge was in hiding / showing appropriate elements in the different states of the application. I overcame the challenges by experimenting with grid, css and javascript.

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

    Feedback on semantic css, and any potential improvements to make the code more efficient.

    Newsletter sign-up form with success message

    2
    Matt Pahuta•580
    @MattPahutaPosted 5 months ago
    • Nice use of flexbox and Grid to match the design comp
    • Good use of semantic elements here (form, ul, etc.)
    • The JavaScript looks great
    • Media queries should be set in rem/em, not px. Here's a good resource
    • I'd consider putting this image in the HTML. Background images are less performant and it's bad practice to have empty divs/elements in the HTML. This particular image is a good use case for the picture element
    • Also with the image: use max-widths instead of hard-coded widths. You shouldn't need to set a specific height property for anything in this challenge either. In general, setting heights like this should be avoided.
    • The email on the confirmation page is not being dynamically applied. You've got solid JavaScript code already, it should be straightforward for you to drop an ID on that span and populate the email received via the form.
    • The cursor for button hover states should be a pointer
    • All in all, very nice work. Cheers!
    Marked as helpful
    0
  • D. Jaime Blockton•180
    @digigrrl525Submitted over 2 years ago
    What are you most proud of, and what would you do differently next time?

    I am most proud of my figuring out how to srcset pictures in the html.

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

    I had a lot of issues getting the white content to line up with the desktop image. To solve the whitespace issue, I set a height for the desktop image at the 1440px breakpoint.

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

    Any constructive feedback is welcome.

    Product preview card

    2
    Matt Pahuta•580
    @MattPahutaPosted 5 months ago
    • Good use of semantic HTML, however, the card's picture and content should not be separate sections. They are elements of the same product card, so better to make the card an individual section (within your main tag) or simply a div and use child divs to create your column/row layout.
    • Similarly, there's no need to set a width on your main tag. The card component should be a child of the main tag. Set a max-width and use a media query on the card component to achieve the desired size of the component.
    • Grace Snow wrote an excellent article on how she would approach writing this project's HTML. I highly recommend checking it out.
    • Also, great use of the picture tag to handle the card image but you don't (and shouldn't) need to set a height. The object-fit property is worth using here as well to prevent image distortion.
    • Related to the image, instead of setting border-radius properties directly, set it on the card (parent) along with overflow: hidden. You can then remove border-radius properties on child elements.
    • You're styling on ID selectors in a few places. This should be avoided as ID selectors can result in very high specificities and create styling conflicts that are difficult to troubleshoot. Use classes or element selectors instead.
    • Don't set a max-width or width on the body element. If you need a way to contain the card component, use a wrapper div. For this project, you could use the main element or, better yet, avoid one altogether. Using flexbox or grid display properties on your body element would work to keep the card component aligned and centered.
    • Your CSS is nicely concise but you ought to use a modern CSS reset for this project (and any in the future). Josh Comeau and Andy Bell both have excellent resets you can incorporate into your CSS file (or use as a separate file).
    • Media queries should be set in rem/em, not px. Here's a great resource
    • You're missing hover and focus states on the button.
    • Overall, very nice work here. With some relatively minor revisions to your code, you'll have a much closer match to the design comp. Cheers!
    Marked as helpful
    0
  • Matt Pahuta•580
    @MattPahutaPosted 7 months ago

    Hi there. Nicely done completing this challenge. You're using semantic HTML landmarks well here and matched the design comp quite closely.

    Here's some specific feedback and tips to carry forward to future projects:

    1. Always use a modern css reset at the start of the styles in every project. Andy Bell and Josh Comeau both have excellent resets worth checking out.

    2. You should use min-height instead of height on your body element. There's no effect for this particular project, but the browser should know it's allowed to grow beyond the viewport height when necessary. Width should not be set on the body. By default, it's 100% and shouldn't be changed.

    3. Likewise, the width and height properties of your main element are not necessary. You could remove all these and put a display of grid on your body to produce similar results.

    4. Media queries should be set in rem/em, not px. Here's a great resource

    5. For your social share component, you're setting base styles using an id selector (#socials) but toggling hidden/show styles using a class (.hidden). I think this is causing some specificity issues and impacting how the card is displaying on different screen sizes in different states. In general, it's best practice to avoid mixing IDs and classes to apply styles.

    6. It's bad practice and an accessibility concern to set a font size on the html element. Instead, set the base font you want on the body or a lower-level element.

    7. Your div with an ID of share-div-2 doesn't appear to be necessary. I think you can omit this markup and the related JS code.

    Good luck with future challenges!

    0