Skip to content
OverviewSolutions
13
Comments
12
P

Aaron Smith

@medic-code260 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
    Anton Vasilache•580
    @Antonvasilache
    What are you most proud of, and what would you do differently next time?
    • Glad I did the HTML/CSS relatively fast
    • I thought about using ASCII codes for a password algorithm and managed to make it work
    • I refactored the functions to pure, and split the code to be somewhat modular
    • Would probably choose a different method or look into a library, if this was something other than a demo project
    What challenges did you encounter, and how did you overcome them?
    • Creating the range input was a challenge, due to the browser defaults - I managed to find a resource that explains well how to remove the browser defaults and create your own.
    • Styling the svg images was difficult due to how they resize
    • Creating the 4 password categories took a lot of trial and error, to cover as many cases as I could
    What specific areas of your project would you like help with?

    I think there could be some more refactoring done to the code.

    Password generator app using vanilla JS

    2
    P
    Aaron Smith•260
    @medic-code

    Hi there,

    General comments Pretty good approach to the challenge. The user experience is good, there's a few minor UI aspects and the logic for too weak, weak,medium and strong passwords was a little difficult to figure out as a user. The HTML and CSS are pretty solid with just a few minor aspects i can think about. The JS was pretty difficult as a challenge, and feel like there's perhaps some room for improvement in the implementation, however you managed to get the user experience correctly.

    Specific feedback

    UI aspects

    1. On first load of the screen you don't have the placeholder present in the password box
    2. The slider is default to 10 when it should be for 0
    3. The checkbox's should have background of green and larger than they currently are
    4. The space between the colours is a bit too close than the design
    5. The copy icon when clicked should display 'COPIED' afterwards - little hard for the user to know if its successfully copied without it

    HTML

    1. Consider using main rather than a <div class="container-wrapper"> - more semantic.
    2. Consider using BEM or another methodology for class names, however the names are not bad - plus using utilities.

    CSS

    1. Try to use relative units for font-sizes, padding, margins (REM) for example rather than px, better experience for responsive design.
    2. The classes names too weak, weak, all have the same properties, can consolidate these into one.
    3. I haven't checked all the measurements in the breakpoints, but i had to change many different properties from the switch from mobile to tablet sizes - so there's perhaps some things to considere here.

    JS

    1. The clipboard function looks pretty decent
    2. The biggest parts to consider are the generating password and generating strength functions, there's a lot of if statements which usually is a sign there is some refactoring to do. 2.1 I suspect you've complicated logic for the strength of the password (I was pretty simple, increasing strength by adding one of the checked boxes) 2.2 Instead of using charcode, have you considered just using a string with the numbers, letters and choosing from them randomly ? The way that i did it is that i choose randomly from the checked list and then randomly again in the specific checked box characters/numbers/symbols.
    Marked as helpful
  • P
    Aaron Smith•260
    @medic-code

    General feedback

    The overall experience is pretty decent, the major UX works as expected apart from the tips, it looks like you need to use the input field for numbers before it works. The HTML and CSS are pretty solid thought.

    HTML

    1. Pretty good semantic HTML, used forms where i didn't so interesting to see!

    CSS

    1. The icons in the input field don't have the padding - i couldn't figure how to do this but thought you should know!
    2. Nice use of CSS grid on the buttons, i hadn't thought about this, used flex and it was a bit more tricky.
    3. The applications isn't quite centered as it is in the design, probably worth adding align-items and justify-content center to the main element

    JS

    1. For a bonus you could try adding in the functionality for the tip % otherwise no comments, pretty solid JS.
  • P
    Aaron Smith•260
    @medic-code

    General feedback

    Generally a good piece of work, nice responsiveness between mobile and desktop views, something i forgot about when completing my solution. There's a few minor UI aspects that could've been corrected. The HTML and CSS are relatively good. The JS i think there's some room for improvement in terms of conciseness, but looks like solid in terms of output on the screen.

    Specific feedback

    UI

    1. On clicking on daily,weekly, monthly i see you haven't highlighted the hrs part of the cards,

    HTML

    1. Some of the class names are a bit non descriptive, perhaps think about using a methodlogy like BEM
    2. Add in the hr's text to the cards as per the design (i can see why you left it out, makes the JS trickier)

    CSS

    1. I noticed that the dashboard isn't placed centrally within the page, you probably don't need to use grid for this on the main/body elements. Grid is better used for aligning complicated parts in 2 dimensions. Not to say i'm not so sure why place-items is not currently centering for you though, looks like there may be a problem with main element inheriting the place-items grid.

    2. Noticed you've got some padding on the body element that might be contributing to it not centering properly

    3. Couple of places you're using hex values instead of hsl, just a very minor point.

    JS

    1. Consider using fetch and live-server to serve your html in VSCode, you've dumped the data into the js file.

    2. The approach of removing and adding classes is probably a little redundant for a simple change in textContent for some elements. 2.1 Consider an approach to change the textContent of the time and last week time elements for each card instead of recreating large amount of the card html.

    Hope some of that is useful!

    Marked as helpful
  • P
    Aaron Smith•260
    @medic-code

    Hi @AinaKhanDurrani

    Here's some feedback

    General feedback Pretty good job at this, the html, css and js are structured well generally and the user experience is nice. There are a few minor aspects to the UX and UI from the designs to take into account.

    Specific Feedback

    HTML

    1. Use <main>, <article>/<section> tag elements for more semantic HTML. In just a small component it may seem a little odd to do this though, certainly feels that way to me when I do these challenges.
    2. Consider using a methodology like BEM for the class names, they're a little non-descript as they are currently.
    3. Instead of using CSS to change the desktop image, you can use the <picture> element instead with a media query.

    CSS

    1. Decent setup and use of variables
    2. Throughout the css you used very specific % for width which probably reflects the need to use max-width in addition to width%.
  • P
    Aaron Smith•260
    @medic-code

    Minor UI

    1. On desktop view you can see the article title is not quite what the design is - check the fontweight, letter spacing and line height.

    2. Similar to last point when you click on desktop view you can see the share element is not quite in the right place as per the design. Check the article preview description for fontweight, letter spacing and line height. They can be pretty particular about these typography aspects you may not have considered.

    3. The name of the author is a little too large, try h4 instead of h3.

    4. On mobile view the title is a little too large and font-weight a little too heavy, it should stretch to only 3 lines use h3 instead and check the typography styles for letter spacing and line height.

    5. Similarly check the typography styles, the letter spacing, as it goes over 4 lines and not 3.

    6. The image in desktop should be overflowing slightly, you can see there's no grey strip on desktop view, its overflowing. I didn't manage to find a way to do this, but just thought i'd let you know about it!

    HTML

    1. You're using main inside an article element, which should be the other way around, the main should go directly below the body.
    2. I'm not sure article is the correct element given that this is a component, i used sections but probably not the best at knowing this myself!
    3. Otherwise looks pretty good, a lot more efficient than mine.

    CSS

    1. You could use a reset for the specific elements together instead of using margin:0 on every h1,h2, h3 including other styles. For example you could use * { margin: 0}

    2. Generally good looking CSS to me! Interesting use of filter, not sure I would've been able to come up with this.

    JS

    1. User experience looks good to me, you caught the edge cases around if you've clicked it in desktop or mobile and then resize it.

    2. The JS is nice a clean and minimal, much more succinct than mine.

    Summary: Overall a pretty decent job, only minor UI details changes. The HTML only has some minor changes i'd make, the CSS is strong and the JS is concise and succinct.

    Marked as helpful
  • Paul Olynek•330
    @ePaulo
    What are you most proud of, and what would you do differently next time?

    Good excuse to review how to set Grid row and column that include spans.

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

    Had to review Grid.

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

    No request for help.

    Responsive Testimonials Grid Section

    1
    P
    Aaron Smith•260
    @medic-code

    Nice HTML structure and usage of px in the grid system was appropriate.

    Nice use of background-property i had not come across these properties before for this.

    Overall good job!

View more comments