Loopstudios Landing Page With React

Community feedback

• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  For code quality I'd highly recommend code splitting. You're functional components are really large, generally a component should be broken down to the deepest level. It looks like you should break down Nav into it's own component, List, as well as list items. You should also use the mapping feature to map list items to a list so you don't have to repeat lines of code. It looks like you're using anchor elements and images enough to break them into their own component. On your hamburger menu the toggling state is overly complicated, as well as bad practice because state is async, so it could not be updated yet when that function is called. The best way to prevent that is just in your function writing setActive((prevState) => {return !prevState}); I'm pretty sure your useEffect hook is wrecking havoc on your performance, you have no dependency array so it's firing every single render, and useEffect is for managing side effects, so setting up the event listener to observe the window size is fine, but you must write a return statement that removes the event listener to clean up the effects. also add a dependency array to the hook containing window.innerWidth, that way it will only fire if that value changes. You could also consider debouncing since you really only want the value at the end of a resize. Also, you're not using props at all? props is one of the most important features in react and should be heavily incorporated in the app composition, instead of manually writing all of your data. Also one last thing, try to avoid styling like you did in the Creations functions. It looks nice but it hits performance heavy, especially an onMouseMove call, react will reconstruct all of the functions and variables at each render unless memoized or placed in a ref, so grabbing directly from the DOM over and over and repainting will cause some performance issues usually.

  Onto the styles.... we've already talked too much about font-size lmao so let's just ignore that. In general your sass is pretty dirty. I'd recommend trying to compile your sass and take a look at the output of the css file, that would probably explain much better than I could. Your nesting is far too deep and often times unnecessary. Try to practice DRY and create a mixin for commonly used styles. If you have sass that's more than 3 levels deep there is something wrong with your method. Also it common practice to put media queries inside of selectors, not the other way around. As well as using em units for the media queries, also line-height should be unit-less, not measured in rem.

  React and sass both have an atomic kind of methodology, so break down components as much as possible, use props and refs, it's ok to have hundreds of React components, and is actually quite common and really shows the beauty of React when you atomize everything.

  Overall tho it looks good, you could try using css modules if you want to try a different method of styling, they're scoped to each component so you don't need to worry about clashing class names.

  3

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @steventoben

  Wow, thanks. I really needed advice for React! I knew the logic for the mobile navigation menu was pretty bad and my Sass was quite ugly, but I didn't bother fixing those things (until now, I guess). 😀

  To be clear, should I split my components into just separate functions or simply create new files for each component?

  1

  

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  @ApplePieGiraffe No problem! If you need any React help I'm probably a good resource since I've been using it professionally for like 3 years so I have a pretty good grasp on most of it. It really depends on the situation, for example on this project here I'd make a Nav.jsx and a List.jsx but in that List component I'd make a ListItem function. If you did that you could also set List.Item = ListItem; Then export the List. Then when writing a list it would look like <List> <List.Item> List Item 1 </List.Item> </List> and that would let you write some cleaner code and keep both List and ListItem functions in the List file. It's ok to have multiple components in a file as long as they're tightly coupled, basically like helper components that wouldn't be used without the main component. Just like you'd never use an <li> element without a <ul>.

  And yeah it's ok to use the nesting feature and it's an amazing feature but people often go overboard and don't think about what that sass code will look like as css code, cause in the end it's going to be converted to css so nesting will create tons of code repetition. It's a great feature but just make sure to think about what the css compilation will look like.

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @steventoben

  Okay, cool! 👍

  1
• 

  Michel Oliveira•230

  @oliveiringPosted about 3 years ago

  hey, I did use display none and block and It works, but I don't if that correct.

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted about 3 years ago

  @oliveiring

  That might work, but I think using the picture element might be a little better for semantics and will allow you to determine when the image changes right in your HTML, which will probably make things a little easier to manage.

  0
• 

  Michel Oliveira•230

  @oliveiringPosted over 3 years ago

  how to replace the img desktop to mobile?

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted about 3 years ago

  @oliveiring

  Hey, Michel! 👋

  I used the <picture> element to change between the desktop and mobile images for the cards in the "Our Creations" section. You can learn more about the picture element and how to use it here.

  1
• 

  zhiwei•55

  @softmagnetPosted over 3 years ago

  A bit late but i think you can use window.matchMedia for js media query instead of using listener in Header.js to constantly track the size of the window and set state every time it changes. I think it is less costly.

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @softmagnet

  Thanks for the tip! 😀

  0
• 

  Elise Rousseau•685

  @SarahHenriettePosted over 3 years ago

  Hi ApplePieGiraffe 😊 !

  I'm just a fan of the hover effect you put on the images in the "our creations" section and also of the effect you put on the burger menu 😃👏

  4

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @SarahHenriette

  Thanks, Sarah! 😊

  3
• 

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  Ah, you beat me to this one! I’m just about finished, but I’m glad our effects are a bit different, so I don’t have to change it up, lol. 😄 Although I did have a double take moment where I thought I was accidentally reading my own code because I also used .header-hero-container. 😂

  Great job, as always! 🙌 The hover effect on the creation cards is really fun with the background text and response to cursor movement. The page also responds really well, and I know how much a pain figuring out the medium/transition styles were - at least for me!

  I know nothing about React, so I’ll nitpick at the design matching instead:

  • The logo is too large on the mobile version, and is also missing from the mobile menu when open. I really like the hover effects on the mobile menu though!
  • The font-weight of the headings should be 300 - slightly too heavy at the moment.
  • The hover on the social icons should be “underlined” like the other nav links if you’re going for design matching.
  • The border overlap in the hero that Em mentioned starts at 340px and happens for about 10px or so.

  Speaking of React, how are you liking it? I’m going over JS in more depth right now, but that was potentially next on my list due to the demand for it.

  2

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @brasspetals

  I knew I was going to forget a couple of small details because I was focusing on React, but you have a good eye, of course, and so thanks for pointing out those things!

  I decided to scrap the underline on hover the social media icons because I liked simply darkening them better. I actually had some other custom ideas for the hover states of those social media icons, but I didn't like the way things were going when I tried them out, so I scrapped them, too. 😅

  Funny thing, I only included the 300 font-weight for Josephin Sans when linking my typefaces from Google fonts, so I didn't bother about explicitly setting a font-weight for the headings, but the browser must store that typeface by default or something because those headings are thick!

  And I changed the z-index of a few elements to get the mobile menu working and apparently forgot all about the logo in the mobile layout!

  React is good, and I like it because it utilizes vanilla JS constructs to accomplish things a little more than other frameworks do (apparently). TBH, I like Svelte better so far because it just makes so much sense (especially if you jump into it after HTML, CSS, and JS), but I have much to learn, as well, so we'll see how it goes!

  Thanks! 😁

  2

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @ApplePieGiraffe I figured the icons were a conscious choice, but thought I'd toss it in there just in case. I had the same font-weight issue when I initially submitted my Insure project. Had to go back and tweak a bunch of layout elements. 🤦‍♀️ Never again.

  Thanks for getting back to me about React (and Svelte) - good to know!

  As always - happy coding, APG! 😄

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @brasspetals

  I just pushed a bunch of those fixes to the live site—so things should be good, now!

  I can't wait to see your solution to this challenge! Let me know when you submit it! 😉

  And, yes, of course—happy coding! 😁

  2
• 

  shahab shafie•70

  @Shhb-Coder-1999Posted almost 4 years ago

  AWSOME WORK !!!!!🙌

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @shahabs789

  TY!

  1
• 

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Very nice 🍎🥧🦒!

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @astroud

  TY! 😀

  2
• 

  Elie Bissor•265

  @ElieB77Posted almost 4 years ago

  Awesome solution !👌👌

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @ElieB77

  TY! 😀

  1
• 

  T

  Grace•31,250

  @grace-snowPosted almost 4 years ago

  I absolutely love that mobile nav effect! Utterly beautiful

  1

  ---

  

  T

  Grace•31,250

  @grace-snowPosted almost 4 years ago

  I hardly touch react, but one of the most common issues I see with react projects is forgetting some of that base html knowledge picked in earlier challenges. That hamburger is a classic example I'm afraid - a div used instead of a button. You know that's gonna make me sad 😉

  But I also know you have the power to fix it ☺

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @grace-snow

  Snap! 🤦‍♂️ I had been careful to use the button element for past hamburger menus I made but I completely forgot about it in this challenge! Thanks for pointing that out! 👍

  1
• 

  Tesla_Ambassador•3,000

  @tesla-ambassadorPosted almost 4 years ago

  This is by far the best loopstudios I have see!!! Cheers to Apple Pie Giraffe. I am in love with the hover effect! Take us to mars bro.

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @tesla-ambassador

  Thanks, dude! Can't wait to see yours! 😉

  1
• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  Hey APG! Great work! The nav transition is pure silk lol and the hover on the creations section is really creative wow!

  I’m viewing this on mobile and I’m getting a small bug where ‘Experiences’ in the header is touching the border of the div. It may just be on ios though, checked with Safari and Chrome.

  Otherwise, this looks pretty much on point as expected 😀

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @emestabillo

  Thank you very much! 😊 I also just noticed that the word "experiences" is very close to the edge of the border around the heading. I think it's that way for all devices at that screen size, but it should be an easy fix. 👍

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @emestabillo

  BTW, I was going to mention that I actually took a bit of inspiration from you for this challenge (such as the responsiveness of the cards in the "Creations" section). 😉 And you should have seen my file structure before I learned a thing or two after looking at the folder structure of your most recent solution (that you made with React) earlier today—mine was a mess! 😂

  1

  

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  @ApplePieGiraffe Haha I deleted part of my first comment before I posted and it went: “I did the exact same thing to my sass files” 😂 I can’t say it’s best practice since I’m still learning but I’ve been studying multipager submissions on FEM and that seems to be one way of doing it. Thanks a lot for taking a look and commenting on my loop project 👍🏼

  1