Skip to content
OverviewSolutions
14
Comments
13
Mariem Ehab
@MaryEhb

All comments

  • Mariem Ehab•335
    @MaryEhb

    The site looks pretty the same as the design and also you are the only one I reviewed who added the hover style to the three dots icons so great job 👏👏 I just have a few remarks:

    • There are more than one element that has the same id 'divsm' and that is not right because the ids are unique and can't be repeated for several elements. You can use classes instead
    • in the same elements as the previous point I noticed that the number of hours change as the user choice but the text (Last week) stay the same where it should also change between (last day - last week - last month)
    Marked as helpful
  • Mariem Ehab•335
    @MaryEhb

    Nice work completing the challenge and the site works well on mobile and functions as needed I just found small things that need modification:

    • The most important one is that when the site is previewed the choice 'weekly' is highlighted while the data that appears is for daily so you should fix it
    • when the choice is changed the boxes disappear for a sec then appear again with the new data which may be annoying for some users
    • the three dots icon should be white when hovered

    Edit: I just saw your code and I really suggest that you don't change all of the inner HTML of the card element each time a choice is selected Instead you can change the inner html or text of the needed elements only which are of classes : title, current and previous

    Marked as helpful
  • Mariem Ehab•335
    @MaryEhb

    You have done a great job this far this design can be really tricky I just have some recommendations hope they help

    • in filter time period class, the list should be selected and colored white when the site is previewed (weekly choice)
    • the vertical spacings need to be decreased
    • the title of boxs is white not gray
    • when boxs are hovered their color should change to a lighter one
    • the color of the three dots icon should be white when hovered
  • Mariem Ehab•335
    @MaryEhb

    Wow! that looks perfect It looks identical to the design and it functions well You are really good 👍👍

    Marked as helpful
  • Mariem Ehab•335
    @MaryEhb

    Well done this design can be a little tricky for some people and I liked how you dealt with the backgrounds of the boxs I just have some small modifications you may have not seen:

    • The frequency div should align its children in a row at small screens
    • the three dots icons color should change to white when hovered on
    • and also don't forget to make the frequency selected item be white and the other choices grey when you add js That's it hope this help and I will be waiting for the active state of the site
    Marked as helpful
  • Mariem Ehab•335
    @MaryEhb

    Great job you have done a fantastic job this far I just have a recommendation on the email input example which I think it will be better if it appeared as a placeholder where the user won't need to delete it before typing his/her email

    Marked as helpful
  • Mariem Ehab•335
    @MaryEhb

    Hello 👋 I just wanted to say nice work 😊 And for more help with the image in the desktop view you can check my solution Hope it helps

  • Mariem Ehab•335
    @MaryEhb

    Wow!!😲 The design comparison screenshot just blew my mind🤯 Great job your work looks perfect👌keep it up (There is just an HTML issue in your report please fix that 😂)

  • Mariem Ehab•335
    @MaryEhb

    Nice work it looks great🤩 It just needs some small adjustments :

    1. The background color is not the same as the design
    2. You should increase⬆️ the grid-gap of the container class slightly
  • Mariem Ehab•335
    @MaryEhb

    I don't know if you previewed the site on an mobile view or not but the text is really squashed in the middle it will be better if you decreased the padding on smaller screens 👍 Otherwise great work😄🤩

  • Mariem Ehab•335
    @MaryEhb

    You have done a fantastic job 😊😃 I just have one comment on the width of the card which is that: • it's not wise to use a fixed width where it may not be responsive in other medias it will be better to make it related to the width of the screen (Ex: width: 95vw) and give it a fixed maximum width (Ex: max-width: 365px) instead so that it can't be bigger than that width in bigger screens but also be always responsive in small ones 👍

  • Mariem Ehab•335
    @MaryEhb

    Nice work keep it up 🐱👍

    I have a weird answer to your question 😂 hope it helps : When I get stuck in that I open the image on figma and write a word that is present in that image then I start comape them by putting the word I wrote above the other and playing with the font size until they be identical and appear as one string

  • Mariem Ehab•335
    @MaryEhb

    That was from the best solutions I saw till now. It's great Keep up the good work :)