Skip to content
OverviewSolutions
7
Comments
9

Vee

@JunasVeeUnited States240 points

Grinding to be a software engineer!

Latest solutions

View more solutions

Latest comments

  • Vee•240
    @JunasVee

    Hi Hussin!

    Your page is responsive and it looks good to me, I can't really mention all of the issues since you said you're still learning reactjs which I think it's common to have difficulties functionalizing particular things. But here are the issues that are such a "big deal" in my personal opinion:

    • The cart, it's one of the main concepts in this challenge. In Ecommerce Product design, having a cart like this makes the customers know what they have been putting on. I saw that you have set the function for it but it doesn't seem working on my device, please let me know if it actually works.

    • Increment and Decrement, I tried to decrease the number when it's 0. it's good that the output doesn't show a negative number, but if you click it 3 times after 0, in order to put it back to 0 it also takes 3 increment clicks, so behind the scene, it is still a negative value without showing it to the output. So instead of creating a string for negative values, when counter <= 0 set its value to 0 too.

    I hope my feedback is helpful.

    Marked as helpful
  • Vee•240
    @JunasVee

    Hi Kohsey!

    I've checked your preview site and its codes in your repository. It looks awesome to me, simple codes only a few lines and the CSS is short as well but everything works fine. Additionally, it's responsive to all screen sizes. I would say that your efficiency is at another level!

    Perhaps the things that you might want to fix are accessibility issues in the report but those issues aren't extremely important. Overall, absolutely well done.

    Marked as helpful
  • Vee•240
    @JunasVee

    Hi Kratos012!

    Congrats on finishing this challenge, I gotta say that your solution looks clean and almost perfect. I'll kick this off by answering your inquiries:

    • I know that this challenge tells you to make the solution looks as close as possible to the example design and probably it includes the font-size, but when it comes to a real-life situation, what actually matters are its readability and its suitability with the web design. This might be a bit off the box but trust me, it would be more useful once you achieve something in this field.

    • px unit is good when a page is made for a single specific viewport and defines the height or width of an element. Most developers use Relative Units such as rem and em for font-size, vh for page's height, and vw for page's width. I personally use % for margin and padding to make it responsive. To fit this particular image, I guess it'd be better to use the img tag instead, I've tried it, and actually worked, additionally, you can make the container's width wider.

    My Feedback:

    • Somehow you made the image looks better and more comfortable to the eyes by blending the background color to it, great job.
    • It seems like the attribution affected your main content's position by pushing it up from the center point. To give the attribution an independent position, set its position to absolute so that it won't affect any other elements.

    The rest is pretty much well done. I hope my feedback is helpful.

    Marked as helpful
  • Vee•240
    @JunasVee

    Hi Faizan!

    Good to know that you've finished your 2nd challenge on this platform, your solution looks good, however, I've found several issues that you might want to fix. Here are the issues:

    • Flexbox, the Main Container should always be both vertically and horizontally centered but in your case, it's not vertically centered even though you've added align-items: center; on the main tag. The problem is, you haven't set the height of the container, therefore it makes the flexbox confused because by default it follows the child element's tallest height. To fix this, add min-height: 100vh; for the main tag and your issue will directly be fixed.

    • Font-Family I suggest you read the style-guide.md file, it says that the font-family should be Lexend Deca but it's not implemented in your solution. To fix this, import the font from the google font website and set it as the font-family for every element.

    • Styling/CSS, We don't need to add a type attribute to a style tag because HTML already knows what it is, removing it would be better because it has no value and is making the code looks filthy. Additionally, it gives you an HTML issue report for this challenge.

    • Main tag, instead of setting a div as the main tag, you can directly use the main tag itself. As a matter of fact, it provides a better Search Engine Optimization(SEO).

    There are actually a few more issues including the texts but I guess it would take too long to type. The point is to keep learning from your mistakes because it's better than not making anything.

    I hope my feedback is helpful.

    Marked as helpful
  • Vee•240
    @JunasVee

    Hi Dawid!

    It is great to know that you finished this challenge, your page looks nice and interesting. However, there will always be room and time for improvements. Here are the issues that I found in your solution:

    • On my screen, the text is big enough and clear, great. But when I tried to open it on bigger screen size, it looks so tiny. Perhaps you can make a media query for a viewport larger than 1800x1000 and increase the content's font-size until it is readable on large screens.

    • I saw that you haven't given your background the properties that it needs. It's duplicating or in the Web Development, we call it background repeating.The size is smaller than most desktop screen sizes as well. In the order to fix that, you can put background-repeat: no-repeat; background-size: cover; background-position: center; on the body tag in CSS.

    • I'm not sure why, but I think there should only be 1 footer tag in each document. To fix this, either wrap the social media components in a div instead of a footer tag or delete the attribution with its footer tag. It makes better accessibility.

    • This is the biggest issue, you didn't add the mobile media query. I suggest you make one because it is really important since everyone has a mobile phone and tends to do anything from their phones.

    The point of these issues is that your page isn't responsive. But it's all back to you whether you want to improve or not, I'm not forcing you, I'm just here to give you the feedback that I think I better give. Keep going 👍

    Marked as helpful
  • Vee•240
    @JunasVee

    Hi DawidMTX!

    I saw that there's no orange box image file in your folder, so I assume that the problem is that the wanted file is missing.

    Your solution works as it should, great. But it seems like it has some issues with the UI/UX or the styling. Here are the things that you might want to fix:

    • If I'm not mistaken, the card's background should be white/#fff instead of grayish-blue as it shows in the design example. But it doesn't really matter since background can be changed easily anytime.
    • The top, left, and right side of the box-shadow is too big that it makes the violet background looks really dark near the card.
    • In large screen sizes the text is too tiny, I suggest you increase the font-size a bit for both headings and p tags.
    • In large screen sizes the FAQs go all the way up to the top of the container instead of standing still at the center just like the design example shows. Since you're using flexbox to position your elements, you can add align-items: center; to the main tag in the CSS so that it'll vertically centers the components in any screen size.

    You provide what the challenge wants you to provide regardless of how the output looks but improving it would be better, Great Job and keep going.

    Marked as helpful
View more comments