Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions1Comments66

All comments

  • P
    Grace• 27,590

    @grace-snow

    Submitted

    No specific questions, but as always feel free to suggest improvements or question why I've done things the way I have.

    I would usually create spacer variables for use on things like font-sizes and paddings, but didn't for this one as it's so small and as I don't have the original design files. This means I am just guessing on sizes - hence random 'magic numbers' in the scss

    Mobile-first 3-column-preview-card example

    3

    Shashi Lo• 1,345

    @shashilo

    Posted

    Great job Grace. I really enjoyed how thorough you were with accessibility. The H1 was a nice touch. I did find some things that I'd like to highlight:

    • The naming convention doesn't follow a true BEM naming. There looks to be nested block names inside elements. EX: c-showcase__card-title
    • I'm not sure why you're using EM units in c-showcase__card-btn when you already have REM set to 16px as the default. This will be a problem if you every nested this component.

    And now for the nit picky items:

    • The width and height of both desktop and mobile are off. Desktop more than mobile. On Desktop, because your solution is smaller, all of the margins and paddings do not match the design.
    • The content inside the card, space between image, title, body, and button are off.
    • The button padding is not accurate.
    • The card body content has an opacity of 0.9 right now, but the design looks much lighter.
    • I'm not sure what justifies the breakpoint at 680px. I'm used to using a common standard breakpoint < 768px which will accommodate for all mobile/tablet screens from 0 to 767px.
    2
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Natalia. This looks pretty good, but I did notice some details that could improve your solution:

    • Paying attention to the details. What I mean is looking at the fonts, line height, shadows, spacing, etc. When you work with a designer, they will be very picky with you implementing it almost pixel perfect.
    • Because you are utilizing Bootstrap, look into the available components it comes with already. One example is the button.
    • The naming convention could improve. If you are follow BEM, I don't see a Block called out. card__component could be a block as card-component. Then anything that is an Element could follow.
    • The proceed and cancel links do not have hover states. This is because there is not a href for them.
    1
  • Richard Verweij• 25

    @Shatango

    Submitted

    I tried to use as less code as possible to not clutter it or use more lines then needed. Are there any ways i could have made my code cleaner?

    Thanks in advance

    HTML/CSS

    2

    Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Richard. I can spot some was you can make this clean cleaner. Here's a list:

    • Usage of modern screen breakpoints. https://getbootstrap.com/docs/5.0/layout/breakpoints/
    • Do not use Id's. Usage of class names here is more ideal. This is a small widget, but when you get into larger scale projects, almost everything is shared or reused. Practicing this will help you down the road. https://css-tricks.com/the-difference-between-id-and-class/
    • Do not append div to CSS selectors. This makes it too specific and harder to override. Simple is best.
    • Using SASS/SCSS or CSS variables to set global variables on reused values like colors, font family, etc.

    Marked as helpful

    3
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Looks really close to the design Anna. The colors look great and the overall implementation is clean. Here's some nit picky things I saw that can improve:

    • When I over lay the design onto your implementation, the size is incorrect. The width is too and tall.
    • Remove the padding top/bottom from the <body>. This is making the window taller than it should be.
    • The icon spacing between the title is slightly off. But on mobile, it needs more work.
    • The breakpoint you have @ 760px should be 768px. Follow these breakpoints for modern screens. Also, if you use min-width instead, it will be a mobile first approach. https://getbootstrap.com/docs/5.1/layout/breakpoints/
    • On your buttons, add border: 2px solid transparent; to the regular state. This way when the hover state comes into play, it doesn't add an additional 4 pixels to the height and width making the elements shift.

    Marked as helpful

    1
  • kevin• 415

    @imonaar

    Submitted

    This is the longest project i have done, so far. Lot of Learning and unlearning. This was done desktop first (which i later realised was a mistake)

    comments:

    1. Still a work in progress, yet to completely fix accesibilty.

    2. Cooordinating the modals was a bit of a challenge, that is why the js is messy.

    3. Still on working on focusing on the modal the specific pledge selected.

    4. working on using localstorage to persist the state.

    comments and corrections are welcome on how I could have done better.

    parcel, netlify

    1

    Shashi Lo• 1,345

    @shashilo

    Posted

    This is a great start Kevin. It definitely looks the part, but can use some touchup. Knowing how particular designers and some companies ware with being pixel perfect, here are some suggestions:

    • Download an image overlay plugin like PixelParrallel. It allows you to see where you're a little off in certain areas based on the design mock ups.

    • Modals background should be darker.

    • Modal options should allow the user to click the entire block to select the option instead of the radio button. This will increase UX and conversion.

    • You did a great job centering the modals!

    • Mobile modal starts in the center of the modal and not the top.

    • Mobile font size and line-height seems off. Definitely the main__wrapper.

    • Mobile menu should have a more subtle transition on open/close.

    • Mobile menu <a> should expand the entire width for it's click state. Increases UX/UI.

    • Make sure logo links to homepage.

    • Instead of using media queries to adjust the mobile screen, build your CSS mobile first. On a slow mobile phone, you do not want the user's device to work twice as hard to render styles. But on Desktop, the user is most likely using a fast internet connection and can handle the additional media queries.

    Marked as helpful

    3
  • P
    Emmilie Estabillo• 5,540

    @emestabillo

    Submitted

    Hi everyone! Happy to be back and submitting another challenge! I started this project to practice React and concepts I’ve been learning from Josh Comeau’s course. I soon realized I was writing the same code for every planet, since every page looks the same, and I thought there MUST be a better way. After some research - enter Nextjs. The dynamic paths seem like the solution I was looking for. That also meant a huge refactor of the entire project. 🤯🤷🏻‍♀️

    My process, struggles, and bugs:

    • The site flickers on mobile so I only enabled the keyframes on desktop.

    • I didn’t try to contain the content into a max-width.I think it still looks ok on a 2k width screen.

    • It was difficult to work around Nextjs’ Image component since the actual <img> tag is nested under two divs with inline-styling. As such, I don’t know for sure if the images are bulletproof. Please let me know if you find any overflow or other quirks with the images.

    • I’m NOT an animator at all lol but I wanted to learn tooling so I tried to work with Framer Motion. It’s a great library but in some instances, it inserts its own transform property which overrides my CSS. I’ve had to modify some of my styles to accommodate, specifically on the images where I was initially using scale to change dimensions. I’d love to know how to also animate the planets when you click on the tabs, aside from animating when the page changes. The library requires a single unique key for the transition, and I’m already passing one for the page change. What’s a different approach to also animate for the tab clicks?

    • How do i make it so that ‘/‘ and ‘/earth’ are both showing the same content? Right now I’m passing the Earth prop to the index page.

    • How do you pass an either-or props to styled components? This code works, but I wonder about bugs.

    Please share your thoughts! Had a great time working on the CSS, the navbar design was a new challenge for me. There’s still some more to do like the readme, cleaning up repeating code, and html warnings but I’ve sat on this “finished” project far too long to not get feedback which resulted to a lot of unnecessary frustration on my end. And that is the big takeaway.

    Planets site with Next.js and styled components

    #accessibility#styled-components#next

    6

    Shashi Lo• 1,345

    @shashilo

    Posted

    I'm just going to review the design and not the React code. Here's some feedback:

    • Instead of showing nothing as the homepage, choose a default and show that. Without a default, it makes the user feel like there's an error with the site. • The level of detail you put into the responsiveness is fantastic! • I don't see the benefit of using CSS Grid in the Header, but it looks good. • Instead of using margin's in the Stats_Item, use gap: 0.6875rem on Stats_Line. • For the main nav, I'd expand the click area to the entire header. It will increase the UX and less chances of the user not being able to click on the menu item. • The plant transitions could be better when navigating through overview, structure, and geology.

    Overall, the site works really well. Great job!

    Marked as helpful

    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Don't be discouraged by this, but let it be a way for you to learn. I'm particular with design reviews because you're going to get grind out by the designer for not paying attention to the details. I hope I'm not too harsh, and let me know if none of this makes sense. Cheers!

    Items to improve on:

    The details. You may not have the Sketch file, but if you overlay the design onto your implement or looked at it side by side, you'll see some differences.

    On Desktop View • Border-radius on the container is off • The content title and body should be left justified • Do not use <strong> unless it's made to be bold or stand out. For this instance, it's misleading because the entire title should be prominent. • Check the font sizes. They look a few sizes off on the title. • The stats column should be split into 3 equal parts and aligned to the start and end of the container. • The stats column label font and font size are incorrect. • The image should have an overlay fade. Right now it's way too bright.

    Responsiveness • If you look at your implementation at 900px to 1400px, the image is cut off. Instead, make the two sections equal in size and I'd start the mobile view below 768px.

    On Mobile view • The design should work on the minimum screen size of 375px wide. Why? Because this is the standard iPhone screen size nowadays. • All text should be aligned centered on mobile. • Please check the padding from the title to the body, from the body to the stats columns, and the stats columns to the bottom. It's all inconsistent and doesn't match the design.

    Marked as helpful

    2
  • Kele 'Yintii' Heart• 185

    @Yintii

    Submitted

    I got it working but backwards.... I either have it to where none of the listings will show until a filter is selected, then the filtered jobs show up. OR all listings show up and the filtered jobs are REMOVED instead of the not-matching jobs from disappearing. Not sure how to get it to go the other way but it's pretty much done, slightly functional lol

    Job Listings - built with React and slight design change

    1

    Shashi Lo• 1,345

    @shashilo

    Posted

    Good work here Kele. I know this challenge takes time to style and implement. There are some things you should clean up, but you should be proud of how far you've gotten.

    • I do like that you added a filter to the top of the page, but if I'm the designer or the product manager, I'd have you remove it until you click on a tag.
    • If you select multiple tags from the filter, it doesn't properly function.
    • On all your clickable states, there's no hover state. This is not good for usability as the user will not know if something is clickable.
    • If you look at the active-states.jpg, it tells you what should be clickable. Currently, you cannot click on the individual job posting tags. That's how a user will filter the results.
    • Font size, letter spacing, padding on tags, missing box-border on each listing, etc. These are details missed.
    1
  • esraagamal• 600

    @EsraaGamal-22

    Submitted

    Rate my design please, Any feedback and suggestions on how I can improve are very welcome!

    using html,css(flexbox)

    3

    Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Esraa. You're getting better at implementing designs, but I do see some areas that you can improve on:

    • The overall container should be max-width: 920px.
    • The method you are using to vertically and horizontally center the project is not ideal. It's not fluid and responsive. I'd remove this and make it margin: auto;. Because you're already using flexbox, add place-items: center;. This will center the elements vertically and horizontally.
    • In the left column, the icon box sizes are inconsistent.
    • Please check the font spacing, white bubble background color, details of the design. There are many little things off about this implementation. Please use PixelPerfect to overlay the design on top of your implementation to view the details I'm talking about.
    • The mobile view is pretty good aside from some padding issues.
    • Look into implementing your CSS mobile first.
    1
  • Cara Uymatiao• 75

    @carauymatiao

    Submitted

    Change logs:

    v2.0: Still have to fix flex boxes so they aren't a static size.

    v2.1:

    • Flex box fixed.
    • Landscape display not accounted for. White box may cover attribution text.

    v2.2:

    • Removed footer div and finally placed image as body bg
    • Fixed landscape orientation problems by converting the whole thing into a flex box

    Flexbox Galore

    3

    Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Cara. Looks like you were able to redo this using Flexbox. Great job! This implementation looks very much like the design too. I did find some areas that can improve:

    • .main should be 920px wide and the child containers should span the entire 920 width.
    • Remove the fixed height from .left-container. Allow this section to be fluid. This way if anything changed, it's easier to know where the fault is. If you inspect this element with browser dev tools, you'll see that the margin cuts the icons in half. Its true size is lost.
    • <div class="icon-bg"> has a hover state, but it's true clickable region is the <img>. For better usability, remove the div and move .icon-bg to <img class="icon-bg">. Less DOM elements, cleaner code, win-win.
    • .progress-bar-active should be 81.5% of the width.
    • Mobile looks great, but the left/right containers should have only 15px spacing between them.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Jisha! You produced a good solution, however I found some things that can improved with your implementation:

    • Font-size and text color needs a closer look. You're setting your headings at font-weight: 500 when they should be 700.
    • Overall container border-radius and box-shadow is missing.
    • The submit button's border-radius and box-shadow are incorrect as well.
    • Your mobile implementation needs work. The body being width: 45% is not ideal. It should be 100% and add margin to the left/right to space the content from the edges.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Shannon. You did a good job including all the elements and styles. I did find some items that can improve:

    • Overall, paying attention to the details. If you get PixelPerfect Chrome plugin, you can overlay the design and see where you are off from the design.
    • The Content padding is a little off.
    • Check the font colors, size, etc. There's a few places that it's a tad off.
    • You're missing the box-shadow from the main container and the submit button.
    • Mobile needs a little work. There needs to be space on the left/right of the container. Also, check the border-radius on Monthly Subscription box.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Paminus. Great job getting this done. It looks good, but I did find some things you can improve on:

    • Overall width should be larger.
    • Font-size and text color needs a closer look.
    • Overall container box-shadow is also off. It's too harsh right now and should be more subtle and spread out.
    • The submit button's box-shadow are incorrect as well.
    • Check the Why Us content. It seems too big and needs more padding.
    • Check your margins on the mobile screen. Some elements need more spacing.
    0
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Very solid implementation Bhanu. It looks very close to the design, but I did find some areas of improvement:

    • Font-size and text color needs a closer look. You're <h2>'s are a little off.
    • Overall container border-radius is too small. The box-shadow is also off.
    • The submit button's border-radius and box-shadow are incorrect as well.

    Other than that, really good job!

    1
  • Romanoli• 265

    @hickkick

    Submitted

    Is it correct to use the input range in this case? is it possible to implement it with a gradient? But please tell me, file icon - to select a file, cloud - to download, why a folder icon? Maybe not a very correct question, and you send me to google) Thank you for your attention! Have a good day!

    Mobile firs, BEM, Flexbox,

    1

    Shashi Lo• 1,345

    @shashilo

    Posted

    Very unique implementation Romanoli. I don't know if this is the right place to use an input range as it's just for show and not an input element, but I like it! I believe there's ways to use pseudo elements to create the gradient, but you'd need to redo the current pseudo elements. The bubble should be a pure white color and I would make the entire icon div clickable for better usability.

    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey John. You did a great job using all the assets. It looks great, but it is missing some details. Here are the areas of improvement that I noticed:

    • You should create a .container with max-width: 1110px and used it within your sections.
    • Overall, paying attention to the detail of the design. For example, the hero section's title font size, call to action button's font size and alignment, etc. If you look at the design or overlay it onto your project, you'll see that you're close, but not pixel perfect to the design.
    • Instead of setting display: flex; to every main section, I'd use display: flex; on the parent and set flex-basis: % to create a width. You can also add align-items: center on the parent instead of each child element.
    • The box-shadow of the section's are too heavy.
    • Footer is missing the huddle image. Also, the content should start below the logo. Missing the social media icons as well. I know there isn't active states, but you should practice making all links real links.
    0
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Emmilie, this looks exactly like the design. I can't even tell the difference! Great job! I did find a few areas of improvement besides the border-radius.

    • The hover states should transition together. Currently, on desktop, when you hover on the CTA, the shared bubble transitions a little later. Onmouseleave, the button transitions quickly and before the shared button. There's no cohesion right now.
    • Missing content box-shadow.
    • The CTA doesn't work well on mobile. Either use an input button and make it pure CSS or use Javascript to toggle the states as the mobile user clicks on the CTA.
    2
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Really good job Jody, but I did find some room for improvement:

    • Background image is in wrong position.
    • max-width of content is too small. Should be 1110px.
    • Name and title should be inlined.
    • Image is missing border-radius. box-shadow is also too much on the image.
    • Position of the arrows vary depending on the viewport screen size. That's not ideal because it's not responsive. I'd position the arrow according to the image container so it's fluid with the design.
    2
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Jody! You did a great job getting making your implementation look like the design. However, I did find some room for improvement:

    • For me, the use of CSS grid here is too much. Seemed like it took a while to get the area correct and placement of elements.
    • Overall width of container is too large. It should be max-width: 920px.
    • You're using font-weight: 700px;, it should be font-weight: 700.
    • Mobile looks pretty darn spot on!
    1
  • Basak ulcay• 110

    @basakulcay

    Submitted

    Hi all, I used responsive design for max-width : 375px but I am realizing there are still issues when I reduced the screensize. Can someone help me out on which other conditions I should be adapting and where else I'm making mistakes? Thanks!

    Fylo data storage component (HTML, CSS, JS)

    4

    Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Basak. I do see more than just responsive issues, but this was a great try. All the elements are used and there just needs to be some cleanup in your code. Here are the things that can improved:

    • They way you're using position: relative and moving the sections is very difficult when it comes to making it fluid. It makes it difficult to be responsive.
    • I would wrap the sections in a .container with a max-width.
    • Remove the set heights from the sections and allow the design to be fluid.
    • Instead of using max-width for your media query, build mobile first. Your default CSS styles are for mobile and use min-width to override styles for larger screens as needed. This is a good article talking about current media queries. http://devfacts.com/media-queries-breakpoints-2020/
    3
  • Shashi Lo• 1,345

    @shashilo

    Posted

    This was a good job Kele. You sure did forget the background, but you didn't miss any of the other elements. I like the animation of the button. It's nice and smooth.

    The details. If you compare the design to your project, it's missing a many details. Here are the items of improvement that I noticed:

    • Annually and Monthly content on desktop is too wide.
    • The entire switch selector should be clickable, not just the circle. This will provide better usability and act like an input button.
    • $ text should be smaller like a subscript.
    • Size of the Professional section is too tall. It's content should align with the other sections as well.
    • Learn More button's should have text-uppercase and they should act as a button with hover state.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Great job Ejiroghene. All the elements are there and it looks very much like the design. Your use of CSS Grid was on point. I did find some areas of improvement though.

    • Pay closer attention to the details. Heading font size and content width.
    • Overall max-width should be thinner.
    • Cards are missing bottom border-radius, box-shadow is too much, the top border should not be a border, instead it should be a pseudo element so it doesn't curve with the border-radius.
    • The entire project isn't horizontally centered.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Good job Shery. Everyone is on point here. The main idea here is paying closer attention to the details.

    • Download PixelPerfect and overlay the desktop-design.jpg on top of your project. You'll see where you are off from the design.
    • Check Font size, line-height, color, font-weight, etc.
    • Size of cards are too big. Need correct border-radius. Box-shadow needs to be toned down.
    • Center your project.
    1
  • Shashi Lo• 1,345

    @shashilo

    Posted

    Hey Deepak. Great try on this. You are missing an important part of the project though. The slides do not slide and are stacked right now. Also, pay closer attention to the details. The font, colors, image border-radius, etc.

    1