Skip to content
OverviewSolutions
12
Comments
17

Shivam

@shivjoshi1996Vancouver520 points

Freelance Web Developer based in Vancouver

I’m currently learning...

ReactJs, Gatsby, NextJS, Styled-Components, React Testing Library, GSAP.

Latest solutions

View more solutions

Latest comments

  • Shivam•520
    @shivjoshi1996

    Hey there! Good job on this one.

    For the dark mode bug, I believe it is because the setState hook queues a render (and only changes it for the next render of the component), it doesn't update the state straight away, hence why you need to click the button twice.

    This might be confusing to read, so I encourage you to check out the new React docs, which helped me understand this too. The specific section where this is explained is https://beta.reactjs.org/learn/state-as-a-snapshot, but I would go through the whole tutorial.

    To fix it, you'll need to separate your logic of checking whether the theme is light or dark, to a useEffect() hook (https://reactjs.org/docs/hooks-effect.html) and check when the [darkmode] variable has been changed.

    something like:

    useEffect(() => { if (darkMode) { document.documentElement.setAttribute("data-theme", "dark"); } else if (!darkMode) { document.documentElement.setAttribute("data-theme", "light"); } }, [darkmode]);

    For the issue with the max viewport , I would remove the max width on the body, and add it to the main content container instead (with how wide you want the component to go), and give it a margin: 0 auto to ensure the content stays on the middle of the screen.

    Hope that helps! Reach out if anything is confusing.

    Cheers

    Marked as helpful
  • Shivam•520
    @shivjoshi1996

    Hey Paras,

    Great job on this. I regards to the background color, I believe there is an image that you should use for it rather than setting a color. It's in the image folder "bg-pattern.svg". If you add that to the body using background-image, set background-repeat: no-repeat and set the size to 100vw 50vh, I think that should work.

    I also noticed that when I clicked on the 25% off toggle it would increase the page views and cost per month x10. I think the correct prices should be listed in the README which you got when you downloaded the files, so if you want an extra challenge then take a look :)

    Marked as helpful
  • Shivam•520
    @shivjoshi1996

    Hey Sai,

    it also looks like your link tag for the CSS is messing things up. Replace type="css/text" with type="text/css"

  • Shivam•520
    @shivjoshi1996

    Hey @JamesTheLessFC, great job on this.

    To respond to a couple of your points:

    1. I've been a Project Manager for a few years working in Software Eng teams and in my experience, I think for someone who is a Junior F/E, 2 and a half weeks is reasonable for this kind of site considering you were working on it part-time. Obviously, if you had a solid 8-hours a day you might've got this done faster, and as you're still learning, the efficiency of how fast you write your code will only increase going forward. So I think in terms of how fast you're getting things done, you're on the right track.

    2. I can't give you a solid answer on the images in terms of best practices (since I'm also learning Sanity & NextJS) but I know Sanity has some special helpers for images (https://www.sanity.io/docs/presenting-images), and there is also a plugin for Sanity + Next specifically. Hopefully that points you in the right direction.

    3. From my personal experience Sanity is pretty great (It's the one I've also used the most while developing since it's the most straight forward IMO). For the content, Sanity would work fine. In a real-life situation, you'll probably be asked to hook the shop up to something like Shopify which can handle things like invoices, order tracking, etc. So while you'll still be able to use Sanity for the content (e.g. page content, product descriptions, images) I assume the actual product management will be handled in another system. That's way beyond the scope of this project though so don't worry too much about that! Some other headless CMS' I've used in my real-life role is Prismic (https://prismic.io/) and Contentful (https://www.contentful.com/) but these are mostly for static sites and not for an eCommerce store. I think if you try a couple of them out, the rest should be somewhat similar.

    4. From what I've read, the React Context API can be used for most cases rather than Redux which has known to be needlessly complex. For a relatively small project like this, I think Context is perfect.

    Great job overall!

    Marked as helpful
  • Shivam•520
    @shivjoshi1996

    Hey Correa,

    Awesome job on this!

    I noticed that when you shrink the window down to tablet size, the time card layout breaks a little, with the background color of the header also expanding down to the bottom. (I tested this in Chrome).

    I think this is due to your grid, if you add align-items: center; to your <main> element, it should fix this issue. Though this might make your profile on the left-hand side shrink in height, but you can use height: 100% on the user div to fix this.

    I also noticed that your time cards have the background colour "bleed" to the edge on the bottom left and right of the cards. I had the same issue and it took me a while to fix, but I eventually ended up making three containers - one parent container for the overall card, one child container for the icon header, and one child container for the time information. This solved the issue, and you can see the code here: https://github.com/shivjoshi1996/time-tracking-dashboard-react/blob/master/src/components/TimeCard.js

    Marked as helpful
  • Shivam•520
    @shivjoshi1996

    Hey there!

    Good job on this challenge!

    I noticed that the circle backgrounds were a little funny when resizing the screen. Using viewport units (vh, which is viewport height, and vw, which is viewport width) may be better. For example, if you replace what you currently have for background-postion, which background-position: right 50vw bottom 40vh, left 50vw top 50vh; it should look a little better.

    I also noticed that your profile card is not completely centred on the screen - this is because your grid container container does not include the footer. Moving the footer into the container div will centre the card. However, you may need to put a bit of padding-top to the footer so that the footer and the card have some space.

    Cheers! Shiv

View more comments