Building a profile image with a fly-out menu in the header

Question

Background

I am more formally trained in backend/server-side coding. Over my career I have often found myself thrown into front-end coding. I have spent the last year and a half learning JavaScript/CSS3 on an "on Demand" basis. I feel like I am getting the hang of it, doing fancier things to meet UX requirements like animations and being able to layout a page without relying too much on a framework like Bootstrap.

Code

In this sample, I have put together an animated flyout menu for a profile image in a header. The idea is, you click on your profile image, and a menu animates to opacity 1, click it again, and the menu animates to opacity 0. For the sake of simplicity, the profile image is simply a cyan circle shaped div . If you would be so kind as to point out any flaws, or better practices I can adopt to make this code more readable/reliable/maintainable, it would be appreciated.

To see a working demo, I have built it in codepen: http://codepen.io/anon/pen/dNxeRN

HTML

<div class="header">
  <a href="#" class="logo">
    My Logo And Brand
  </a>
  <div class="profile-image">
  </div>
  <div class="fly-over">
  <ul>
    <li>Account settings</li>
    <li>My Profile</li>
    <li>Logout</li>
  </ul>
  </div>
</div>

CSS

*{
  margin: 0;
  padding: 0;
  font-family: helvetica, arial, sans-serif;
}

.header {
  padding-top: 5px;
  width: 1000px;
  height: 60px;
  border-bottom: 1px solid darkgrey;
  position: relative;
}

a.logo {
  display: inline-block;
  font-size: 25px;
  margin-top: 14px;
  margin-left: 10px;
  text-decoration:none;
  color: black;
}

.profile-image {
  width: 50px;
  height: 50px;
  background-color: cyan;
  border: 2px solid grey;
  border-radius: 50px;
  cursor: pointer;
  float:right;
  margin-right: 50px;
}

.fly-over{
  position: absolute;
  display: none;
  width: 300px;
  border-radius: 5px;
  box-shadow: 1px 1px 10px 1px rgba(0,0,0,.3);
  text-align: left;
  right: 36px;
  top: 80px;
}

.fly-over ul, 
.fly-over ul li {
  margin: 0px;
  padding: 0px;
}

.fly-over ul li {
  list-style-type: none;
  padding-left: 20px;
  padding-top: 10px;
  padding-bottom: 10px;
  border-bottom: 1px solid lightgrey;
  cursor: pointer;
  transition: color, background-color 333ms;
}

.fly-over ul li:hover{
  background-color: #888888;
  color: white;
}

.fly-over:after{
  content: " ";
  display:block;
  position: absolute;
  width: 0px;
  height: 0px;
    border-left: 10px solid transparent;
    border-right: 10px solid transparent;
    border-bottom: 10px solid grey;
  bottom: 100%;
  left: 250px;
}

.fly-over.show{
  display: block;
}

.animation-fade-in, 
.animation-fade-out{
  display: block;
  animation-name: fadeIn;
  animation-duration: 333ms;
  animation-timing-functon: ease-out;
  animation-fill-mode: forwards;
  animation-direction: normal;
}

.animation-fade-out{
  animation-direction: reverse;
}

@keyframes fadeIn{
  0%{
    opacity: 0;
  }
  100%{
    opacity: 1;
  }
}

JS

var el = document.getElementsByClassName("profile-image")[0];
var menuState = {
  visible: false
};
el.addEventListener('click', function(){
  var flyOverEl = document.getElementsByClassName("fly-over")[0];
  function onFadeInAnimationEnd(){
    this.classList.add('show'); 
    this.classList.remove('animation-fade-in');
    menuState.visible = true;
    this.removeEventListener('animationend', onFadeInAnimationEnd);
  }
  function onFadeOutAnimationEnd(){
    this.classList.remove('animation-fade-out');
    menuState.visible = false;
    this.removeEventListener('animationend', onFadeOutAnimationEnd);
  }
  if(menuState.visible){
    flyOverEl.classList.remove("show");
    flyOverEl.addEventListener("animationend", onFadeOutAnimationEnd)
    flyOverEl.classList.add("animation-fade-out");
  }
  else{
    flyOverEl.addEventListener("animationend", onFadeInAnimationEnd)
    flyOverEl.classList.add("animation-fade-in");
  }
})

Answer 1

I've reviewed your code and I can make the following remarks:

CSS rules

I dont find your css particularly bad its just you need to think how the rules can fit each other logically with the css class names you assign to. Lets go in depth:

.header {
  width: 1000px;

Why did you hardcode 1000 px here? If you don't know the width sizes of the screen you want to target its better if you give it full width like width: 100% and if you want some minimum widths so things will not collapse you can add a rule like min-width: 700px

.animation-fade-out{
  animation-duration: 333ms;

.fly-over ul li {
  transition: color, background-color 333ms;

I would certainty reduce the animation duration of those 2 to be no more than 0.2s.If you notice the transitions are too fancy and they don't look appealing to the eye. Use a smaller duration to make them smoother.

.fly-over{
  display: none;
  width: 300px;
  border-radius: 5px;
  box-shadow: 1px 1px 10px 1px rgba(0,0,0,.3);
  text-align: left;

  position: absolute;
  right: 36px;
  top: 80px;
}

My main concern on this class is the last 3 rules about positioning. Why did you choose to mix positions with decorations in the same class? It makes it more difficult to reuse its a one off. It may not be bad for small apps but for larger ones this really counts.

If I were to took they fly-over class and use it on another element I would have to override the positioning with a new class which would add too much bloat. A better solution is to make this class a little more generic and name it like fly-over-menu and add another class a little more specific named profile-menu with the positioning rules. That way I can reuse the fly-over-menu in other places.

background-color: #888888;
color: white;

I don't like mixing color code names with hex values. Pick one preferred representation and stick with it. That way the code will be more consistent.

Now lets focus on the Javascript:

Global state pollution

var menuState = {
  visible: false
};

Why did you choose to save state here? As you will read bellow the state actually is saved in the element classlist. If the element has a class show its visible otherwise its hidden provided that it has display:none by default.

And one other thing What are you saying here is hey I want to save this object to the global namespace and add one property visible to it. You will literally have window.menuState = {visible: false}; Why do you want to save something so specific in a global object?

You really don't want to do that. What if you had 10 of those menus? You had to change the representation of the state object every time. And if you wanted more variations of it like poping-menu or something you would have to add more objects. State management in Javascript is still behind so try to be a little bit more resourceful with what you have.

DOM manipulation

el.addEventListener('click', function(){
  var flyOverEl = document.getElementsByClassName("fly-over")[0];
  function onFadeInAnimationEnd(){
    this.classList.add('show'); 
    this.classList.remove('animation-fade-in');
    menuState.visible = true;
    this.removeEventListener('animationend', onFadeInAnimationEnd);
  }

  function onFadeOutAnimationEnd(){
    this.classList.remove('animation-fade-out');
    menuState.visible = false;
    this.removeEventListener('animationend', onFadeOutAnimationEnd);
  }

 if(menuState.visible){
    flyOverEl.classList.remove("show");
    flyOverEl.addEventListener("animationend", onFadeOutAnimationEnd)
    flyOverEl.classList.add("animation-fade-out");
 }
 else{
    flyOverEl.addEventListener("animationend", onFadeInAnimationEnd)
    flyOverEl.classList.add("animation-fade-in");
 }

})

Oh boy. I don't mind using plain javascript for simple things but did you notice while typing this code that you are repeating yourself? I did just by reading it. What you are really doing here is this. You just toggling class names of the fly-over element depending if the element is visible or not. You also hook the animationend events that are getting fired once the animation is completed to just clean up the event listeners.

I don't think its too bad but to be honest I believe its too low level. You want something more abstract and simplified when dealing with DOM events and callbacks. so to have time to work on more interesting things. If were you I would use jQuery to handle all of this staff don't get me wrong but also I think its safer.Its more of a personal preference this one so you can ignore it.

However check out this line:

var flyOverEl = document.getElementsByClassName("fly-over")[0];

You are lucky here that you have only one element with that class in the document. If you were to add more than one you would see flying ponies.

Also note that the query selector for the fly-over is the only specific thing here and its better if its passed as a parameter using bind or wrap the function in a wrapper and apply the callback with the arguments. That way you can pass whatever class you like in the event callBack. I will leave that as an exercise.

In general I think you are working it well its just some peculiarities of the front-end development you need to understand better.