Skip to content
OverviewSolutions
32
Comments
12
P

Matt Pahuta

@MattPahutaCosta Mesa, California670 points

Howdy! I'm a software engineer currently focussed on sharpening my front-end web development skills.

I’m currently learning...

Sass, 11ty, React, Node, Express, and MongoDB

Latest solutions

View more solutions

Latest comments

  • P
    Felix•300
    @felix-stuff
    What are you most proud of, and what would you do differently next time?

    I am proud that it's finally working properly and the password is generated properly by the selected criteria. I'm also happy about the added checks and features that improve the UX.

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

    Most challenging was generating the password properly and making sure all criteria was acknowledged without overcomplicating the code.

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

    Maybe how to reduce the mess of selectors in the JS to improve the interaction between dynamic code and static markup.

    Vanilla JS Password Generator

    1
    P
    Matt Pahuta•670
    @MattPahuta

    Hi there, Overall, your solution is a solid match to the design comp with good functionality and responsiveness. I particularly liked your use of proper semantic elements along with effectively deploying screen reader-specific styling to enhance the accessibility of un-labeled elements. Also, I appreciated your clean and organized JavaScript code, as well as the comments explaining the logic.

    I didn't experience any obvious errors testing your app but I did flag a few things you might consider for future revisions:

    • Your password output field allows for copying before a password is generated. You could add some validation logic to prevent this.
    • On page load, the idle state of your "Generate" button doesn't match the design comp. The background color should be the neon green.

    As for cleaning up some of the selectors/global variables, some things to consider:

    • You could avoid defining checkboxes since you're only using that variable once (applying the listeners with the forEach loop). Chaining the forEach method to the selector would work just the same. However, since those checkboxes are form elements, you can avoid adding individual event listeners altogether. You might consider using the FormData constructor instead to gather up those selected values. This would likely simplify your JS logic a bit as well.
    • The pwStrengthIndicator and pwStrengthResult variables look like they're only used in the calcStrength function. You could make those locally scoped within the function.
    • You could also skip the form variable definition and simply chain your listener to the the getElementById selector you're using.

    Again, nicely done completing this challenge. Cheers!

    Marked as helpful
  • P
    Matt Pahuta•670
    @MattPahuta

    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
  • P
    Matt Pahuta•670
    @MattPahuta

    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!

  • P
    Matt Pahuta•670
    @MattPahuta

    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
  • P
    Matt Pahuta•670
    @MattPahuta

    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!

  • P
    Aaron Smith•260
    @medic-code
    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
    P
    Matt Pahuta•670
    @MattPahuta

    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!

View more comments