Skip to content
OverviewSolutions
24
Comments
28
Michael Bishop
@MikeBish13

All comments

  • Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Incredible job on this - nothing but praise! Absolutely love the animations you've added. I've never looked at Framer Motion before so will definitely check this out for my next project!

    Great work!

    1
  • Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project - everything seems to be working well.

    Couple of things I've spotted in terms of how you've built your React app:

    • It's best practice to not use DOM selectors when using React (eg document.getElementByID), as this defeats the purpose -- React was designed so that you wouldn't need to access the DOM directly and that it could be manipulated based on state. For example, in order to make your cart visible, you could create a local bit of state cartIsVisible which could either be true or false, then create a CSS class for the cart isVisible, which would have the property of display: block, then put a conditional statement in your cart's className, eg className={cartIsVisible ? 'cart isVisible' : 'cart'} so that every time cartIsVisible is changed, the display property would also change.

    • Secondly, you should also try not to have onClick and 'onChange' functions written out fully within your JSX -- it can get quite messy and is difficult to understand. Instead, create a name for the function and write it out above the JSX, then reference it below.

    Hope that all helps!

    Marked as helpful
    0
  • Chitrang•150
    @cwebdevSubmitted over 2 years ago

    Hi,

    This is my solution for Huddle Landing Page.

    I was unsure of how to implement social media icons but I downloaded some SVG from internet and used CSS filter property to change color on hover. Is it a good approach or is there some other way?

    Any other general feedback is welcome.

    Happy Coding!

    Huddle Landing Page

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Good job on this project!

    In answer to your question about SVGs: if you want to style them on :hover then the best thing to do is actually insert them into your code, rather than use an img tag with a src attribute.

    The beauty of SVGs is that they are actually built with code, so you can access and change different properties without having to overlay colors onto them, as you've done here. It'll make things so much easier for you. For example, you can usually access an SVGs outline through:

    svg {
    path {
         stroke: blue;
    }
}

    Check out this link to find out more about styling SVGs.

    Marked as helpful
    1
  • ERIGO Chigozie Nicholas•650
    @nick335Submitted over 2 years ago
    • how was the right top border of the hamburger menu styled to face up??
    • how do I make my code responsive with less amount of code? (feeling like I wrote a lot of code for this project which could have been avoided)
    • how do we deal with positioning in terms of responsiveness, are we always going to change the position directly for different sizes of phones as I did?

    html, javascript, grid, flexbox

    2
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project. Your solution matches up pretty well to the design, so well done.

    I've tried to answer your specific questions below:

    • I'm not sure what you mean by the top border of the hamburger menu, but happy to take a look if you can clarify or expand on the question

    • The easiest route to better responsiveness/less code is to style your page mobile first, ie style the mobile design first and then work upwards by including @media statements with a min-width. You'll find that in the long run this will save you a lot of code. There's also another reason for you using a lot of code in this project - see below

    • I think you're making this project a lot more difficult for yourself by using generic CCS classes for everything, and then trying to shoehorn the design into your CSS by using very complicated CSS selectors, eg #grid-section .flex-container:nth-child(3) .card:nth-child(1) .display-content h3. With a few extra classes added to some of your components, the styling would be a lot easier for you to code and a lot easier for somebody else to read. For example, the third section of your grid-section is stylistically very different to the other two sections, so why not give it some extra classes, rather than you having to use absolute positioning? For what it's worth, I think this project could easily be complicated without using a single position: absolute.

    Hope that helps and keep up the good work!

    Marked as helpful
    0
  • Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on the project.

    Few helpful pointers for you:

    1). Maybe have a look again at the text within the box and see if you can match it to the design - consider font-size font-weight line-height and text-transform

    2). The site is responsive and the layout changes on different screen sizes, so well done on that. Again you should pay attention to the smaller details - at 375px the box takes up the whole screen to you may want to consider adding some padding around it. Similarly, your first breakpoint is at 425px, which is way too small for the two-column grid layout. Maybe consider increasing this to somewhere around the 600/700px mark.

    Hope that helps!

    0
  • abhikr4545•280
    @abhikr4545Submitted over 2 years ago

    When I click remove on tag the list does get updated instant rather it waits for next render. Any help would be very useful. Thanks

    Solution using react

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on the project - looks really good!

    In answer to your question - the issue that you have is that you are using jobList for everything and so this keeps getting overwritten with new pieces of filtered data and so you can never return to the initial list of jobs. What you need is a separate array/piece of state for your filtered results. This is how your app currently works:

    1. jobList is populated with data and this loads on the page. All good so far.
    2. 'Frontend' filter is clicked and jobList is filtered accordingly. So your filter is working up to this point.
    3. 'Frontend' filter is unclicked and so your filter removes everything in jobList, which leaves you with an empty array.
    4. Now when you try to display all of your items again using jobList, you're doing so from an empty array and so nothing is displayed.

    Hope that helps!

    Marked as helpful
    0
  • Shaina Khan•220
    @shainaySubmitted over 2 years ago

    Feedbacks are always welcome. let me know where i messed up Happy Coding..!!

    Testimonials Grid

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project!

    A few things to point out:

    • Each of the boxes overflows on the screen at mobile screen sizes. Have a look at the size of the content of each box, which is causing the grid to get larger to accommodate it.
    • This problem could be because you've coded this desktop-first, whereas mobile-first quite often makes things a lot easier. For example at mobile screen sizes you're using a lot of !important rules on your grid layout, which isn't best practice. This should be a last resort only, so try tackling the problem from a different angle first.
    • Your footer is also inside the grid container and therefore becomes a grid item, which causes it to pop up in unusual places at certain screen sizes.

    Hope this helps and keep up the good work!

    0
  • Yash•130
    @yashviradiaSubmitted over 2 years ago

    Hi everyone,

    In the beginning, I used only CSS positioning, and it made progress on the project cumbersome. Then with help of CSS flex box, it got easier.

    Feedback on code review most welcome!

    NFT preview card component

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project!

    One small point: you've used a <p> tag to create your image overlay, which is bad practice when we talk about semantic HTML and how browsers, search engines, screen readers, etc interpret your page. A <div> would probably be better suited here.

    Better still, you could maybe use a pseudo element to create the image overlay which could also hold the image in the content property - that way you don't need to use 2 elements and have 2 hover selectors.

    Give it a try! Best of luck!

    Marked as helpful
    2
  • Pawn•320
    @CryptoPawnSubmitted over 2 years ago

    Feedback on my JS is greatly appreciated. Also, if reading from a .json file, is async functions always needed? Or would it be possible to convert a .json file to a JS object without using async functions in $(document).ready

    Challenge #18 - Time tracking dashboard

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project!

    In answer to your specific question -- generally yes, when fetching data from an external file or an API, this needs to be done asynchronously because the fetching can take some time (maybe the server is being slow, for example) and as Javascript is single-threaded, everything else will need to wait until the data is fetched. Here's a nice article explaining it.

    As for more general feedback on your JS, though your solution works it seems to be very prescriptive, ie you have assigned variables in your JS file (the cardNames, for example) which essentially dictate how the data is displayed on the screen, and this isn't very flexible.

    Say that we wanted to add a new activity ('Coding') into the grid and we added the corresponding data to the data.json file. You would need to go into your JS file and edit it, rather than the JS file being able to handle the new data and display it accordingly. Similarly, say there were 200 activities that needed to be displayed - you would have to manual write them into your code.

    This is why it would be better to automatically fetch and then display the data dynamically.

    Hope all that makes sense, and keep up the good work!

    Marked as helpful
    1
  • John Andrey Galingan•180
    @johnrookie-coderSubmitted over 2 years ago

    I think I happened to mess things out with the layout. I also don't know how to add the flipping effect. Any comments are highly appreciated. Thanks.

    HTML, CSS AND JAVASCRIPT

    1
    Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this project!

    Design looks good and the countdown works as expected.

    The obvious thing to point out is the missing 'flip clock' function. It's actually quite a tricky thing to implement and requires a good working knowledge of CSS -- but it's also a brilliant learning opportunity and a good way of getting to know some advanced CSS techniques.

    My advice would be to take a look through some of these examples on CodePen of how others have implemented it and once you've fully understood how it works (this is the important part!) then try and give it a go yourself.

    Good luck!

    Marked as helpful
    0
  • Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great effort on this challenge!

    In terms of functionality, the main issue I see is that I'm able to select all of the numbers and then submit the form, whereas the brief states that only one number should be selected and submitted. See if you can figure out a way of getting this to work.

    Another thing I've spotted is that you've used camelCase as a naming convention for your CSS classes. The standard naming convention for CSS is hyphenated (eg. user-name, user-id, etc) so I'd suggest adopting this to save yourself a lot of confusion in the future. Here's a good article explaining a bit about naming your CSS classes, and how BEM is a very useful and widely used convention. Maybe give it a try in your next project!

    Keep up the good work!

    Marked as helpful
    1
  • Michael Bishop•1,080
    @MikeBish13Posted over 2 years ago

    Great job on this - the game seems to function as expected.

    One major issue I have is that I can't scroll down on the screen, so the 'rock' icon is only a quarter visible. Seems like you've set overflow: hidden to the .root selector which is causing the issue.

    Keep up the good work!

    Marked as helpful
    0