Skip to content
OverviewSolutions
7
Comments
3
AdrianH
@Adrian-py

All comments

  • AdrianH•190
    @Adrian-pyPosted over 2 years ago

    Hello there! First of all, good job on the solution. Although I do think that there are several things that I feel can be improved:

    1. Responsiveness After viewing the solution in several different resolutions, I noticed that it was not responsive enough. Although I did notice that you did do some changes for screens below 600px in your CSS, I still think it's better to also prepare for screens both larger and smaller than 600px. Some resolutions that you might want to code for are: a) 1920 x 1080 (Large Screens) b) 1440 x 900 (Medium Screens) c) 768 x 1024 (Tablet Screens) d) 360 x 640 (Mobile Screens)

    There are other screen sizes, but you can start by preparing for those and maybe start to do more in the future.

    1. Hover, Active, and Pointer Effects What I mean by this is that maybe you could add hover effects when a button is hovered or "active" effects when your buttons are clicked. Furthermore, you could also change the buttons' cursors to be pointers, this can be done just by adding "cursor: pointer" in the CSS. If you want to learn a bit more about hover and active events in CSS, here is a link you can refer to https://www.w3schools.com/csSref/sel_hover.asp.

    That is all from me, overall good job on the solution, and keep up the good work!

    Marked as helpful
    1
  • AdrianH•190
    @Adrian-pyPosted over 2 years ago

    Hello there! First of all, I really like your solution to this project. After going through the source code and also checking out the live site itself, I still feel that there are several things that can be improved.

    1. File Organization When I first opened the GitHub repository, I saw that you placed all the CSS, Javascript, and SCSS files inside the assets folder. In my opinion, you should not do that, instead, you should create a separate folder to put your files in. In my experience, the assets folder only stores images, audio, or videos that are used.

    2. Separating SCSS Modules The way you separated the modules for "globals", "mixins", etc. is already really good, but I just want to suggest another way of separating your SASS/SCSS modules. Here is a link to a Github repository made by a content creator on Youtube called CoderCoder: https://github.com/thecodercoder/fem-dklt-toggle

    That is all from me and I am very sorry if there are many mistakes on my part.

    1
  • AdrianH•190
    @Adrian-pyPosted over 2 years ago

    Hi there! First of all, I really like your solution, there are several things that I really like about the solution but there are also things that I feel that can be improved.

    Some of the things I really like:

    1. The naming of the classes for each element is spot on. There is no over-complicated nesting on the naming of each class, instead, you named each element with the block name (the first class in the name) as the anchor. I really like it because it makes it easier for you to code your CSS or in this case SASS, without having any overcomplicated nesting on the SASS files, I really like it and would also follow it in the future.

    2. The smooth animation for the images. I love the smooth animations you have for the product images, especially the infinite image carousel on mobile screens and the lightbox for desktop screens.

    3. The shopping cart logic. I like the fact that if you add the item twice, instead of treating it as two different products, it stacks it whilst also adding it and checking whether it has reached the max number of items. I really love the idea and good job on implementing the logic for this.

    Some of the things that I feel can be improved:

    1. Separation of the SASS files. I saw that for each file on each module/folder you import it one by one to the main.scss file. In my opinion, this is not a good way to structure your SASS files. Instead, maybe you can structure them more like so https://github.com/thecodercoder/fem-dklt-toggle/tree/main/app/scss, it provides separate central files for each module/folder and import them to the main.scss.

    2. Some unresolved accessibility issues. I also noticed that there are some accessibility issues like not having alt tags on images and buttons not having texts inside of them. I would strongly suggest trying to resolve some of these issues. For the missing alt tags, I would say that alt tags are not necessary for <img> tags that are used for icons but are necessary for actual images.

    3. Using @import instead of @use or @forwarrd for the SASS. It is better to use @use or @ forward for separating SASS files, as the SASS team discourages the use of @import. Here is a link in case you want to read more about how to implement @use and/or @forward.

    Marked as helpful
    0