Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Front-end (Code Review) #78

Open
jamiecoe opened this issue Oct 14, 2018 · 1 comment
Open

Front-end (Code Review) #78

jamiecoe opened this issue Oct 14, 2018 · 1 comment
Assignees

Comments

@jamiecoe
Copy link

Compressing your images

Your current background image (a beautiful as it is) has a whopping size of 9.15 MB! Remember that people using your site on mobile and/or with a bad internet connection are going to struggle downloading content. You therefore want to keep your file sizes as small as possible.

You can definitely reduce the resolution of the image for a start, maybe have a research of what other people are using. Then I'd highly recommend using https://tinypng.com/, a free website for compressing your images (without a noticeable drop in quality). Note that the images need to be below 5 MB before you can start compressing them.

header.js

  • Usually the convention is to name the file the same as the component eg: Nav.js, it makes things more intuitive to find for an outside developer.
  • Having a method called navBar isn’t very descriptive, you want to name it based on what it’s doing. A better name would be toggleMenu.
  • Rather than having two properties in state that update when you click the button, why not just have one and use that to set the properties? The problem with having two state properties for essentially the same thing is that you risk them going out of sync. Better to just have one property that controls both. Eg:
class Nav extends React.Component {

   state = {
       menuOpen: false,
   }

   toggleMenu = () => {
       this.setState((prevState) => ({
           menuOpen: !prevState.menuOpen
       }))
   }

   render() {
       return (
           <nav className="nav">
               <div id='navDiv' className={this.state.menuOpen ? 'menu-open' : 'menu-closed'}>
                   <Link to='/'>Home</Link>
                   <Link to='/categories'>Social Actions</Link>
                   <Link to='/create-event'>Create Event</Link>
                   <Link to='/categories'>Categories</Link>
                   <Link to='/eventsByTheme'>Events By Theme</Link>
                   <Link to='/Faq'>FAQs</Link>
                   <Link to='./displayEvents'> display Events </Link>
               </div>
               <img className={this.state.menuOpen ? 'image-open' : 'image-closed'} onClick={this.toggleMenu} src={ menuImg }/>
           </nav>
       )}
}

export default Nav;
@jamiecoe jamiecoe changed the title Front-end (code) Front-end (Code Review) Oct 14, 2018
@MissArray
Copy link
Collaborator

Image size

I'm definitely on the case of image compression - hard not to notice the 9+ MB! The image is what they're using on their existing website, which we've been asked to base our MVP on. Currently working on finishing a couple of pages and have left the image compression for later.

Headings

Already changed two branches ago, but the PR has yet to be merged; well-spotted.

File names

The view from the back end is that changing file names and component names would mess things up. Agree that they'll make little sense to anyone checking our code; the files were set up while waiting for our clients to brief us, so the names are guesses, really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants