5
\$\begingroup\$

I write a web app that shows events on a multi room scheduler. I use VueJS for the first time on a real project.

Here, I loop through the events array every room column (two nested v-for). In jQuery I need to loop once through this array to fill all columns at once.

My question: is there any other more efficient way to write this code that fills rooms divs with events in Vue?

var vm = new Vue({
        el: '#app',
        data: {
            rooms: [
               {id: 1, title: 'Room 1' },
               {id: 2, title: 'Room 2' }
            ],
            events: [
              { id: 1, start: 230, duration: 30, title: 'Event 1', room_id: 2},
              { id: 2, start: 400, duration: 45, title: 'Event 2', room_id: 1}
            ]
        }
    })
.agenda-wrapper { max-width: 100%; overflow-x: scroll; }

  .agenda { padding-top: 50px; overflow: hidden; }
  
.room {
    width:300px;
    height:1170px;
    padding: 0 10px;
    margin-left: 5px;
    background:#ECECEC;
    position:relative;
    float: left;
}

.room span {
    position: absolute;
    top: -20px;
    height: 10px;
    text-align: center;
    width: 300px;
    font-weight: bold;
}

.event {
    display: block;
    background:#fff;
    color:#4B6EA8;
    border:1px solid #ccc;
    border-left:4px solid #c4183c;
    position:absolute;
    padding:0 10px;
    font-weight:bold;
    font-size:13px;
    border-radius: 5px;
    width: 280px;
}
<div class="agenda-wrapper" id="app">
  <div class="agenda" :style="{ width: (rooms.length * 350) + 'px' }">
    <div class="room" v-for="room in rooms" :key="room.id">
      <span>{{ room.title }}</span>
      <div class="event" 
        :style="{ top: event.start + 'px', height: event.duration + 'px'}" 
        v-for="event in events" 
        v-if="event.room_id === room.id">
      <h5>{{ event.title }}</h5>
    </div>
  </div>
</div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.5.17/vue.js"></script>

\$\endgroup\$
  • \$\begingroup\$ Hi and welcome to Code Review! I'm planning on answering your question later, but just out of curiousity: How would you do it in jQuery? \$\endgroup\$ – Simon Forsberg Mar 5 at 8:44
  • \$\begingroup\$ Hi, thank you for your reply! I think it would be something like that: '$.each(events, function(event){ var el = $("<div></div>").addClass("event").css({ "height": event.duration + "px", "top": event.start + "px" }).text(event.title); $('.room').data('place', event.place_id).append(el); });' \$\endgroup\$ – Alex Mar 5 at 9:02
  • \$\begingroup\$ I also think that maybe I need to do it on backend to make a rooms array look like that: { id: 1, title: 'Room 1', events: [{ id: 1, title: 'Event 1', start: 90, duration: 30 }] } \$\endgroup\$ – Alex Mar 5 at 9:03
  • \$\begingroup\$ Yes, backend is Laravel app. It has two models: Room and Event. Event belongsTo Room. \$\endgroup\$ – Alex Mar 5 at 13:34
3
\$\begingroup\$

I asked you how you would do it in jQuery and you said that you would do something like:

$.each(events, function(event){
    var el = $("<div></div>")
        .addClass("event")
        .css({ "height": event.duration + "px", "top": event.start + "px" })
        .text(event.title);
    $('.room').data('place', event.place_id).append(el);
});

And sure, this would only loop through the events array once. But what happens in every iteration? It does a jQuery search for $('.room').data('place', event.place_id), which is not a constant-time lookup operation, so this approach might be slower than what you think it is.


Now, for your Vue approach. You said:

maybe I need to do it on backend to make a rooms array look like that: { id: 1, title: 'Room 1', events: [{ id: 1, title: 'Event 1', start: 90, duration: 30 }] }

And yes, I agree with this. Putting the events inside your rooms makes much more sense.

As for your Vue template, overall the code looks fine. I'm not a CSS expert so I don't know if there's something you can improve there, but there probably is. Maybe consider using CSS Grids, where each room could be one column, and time-slots could be rows? The current approach of having 1px = 1 minute seems a bit unnatural to me.

As for this part:

<div class="room" v-for="room in rooms" :key="room.id">
  <span>{{ room.title }}</span>
  <div class="event" 
    :style="{ top: event.start + 'px', height: event.duration + 'px'}" 
    v-for="event in events" 
    v-if="event.room_id === room.id">
  <h5>{{ event.title }}</h5>
</div>

The only thing I would change is to put v-for and v-if before your :style-binding, as they are more important to be aware about, this is mostly my personal opinion though.

<div class="room" v-for="room in rooms" :key="room.id">
  <span>{{ room.title }}</span>
  <div class="event" 
    v-for="event in events" 
    v-if="event.room_id === room.id"
    :style="{ top: event.start + 'px', height: event.duration + 'px'}" 
    >
  <h5>{{ event.title }}</h5>
</div>

It would be nicer if your events would be inside the rooms, as then you could write:

<div class="room" v-for="room in rooms" :key="room.id">
  <span>{{ room.title }}</span>
  <div class="event" 
    v-for="event in room.events" 
    :style="{ top: event.start + 'px', height: event.duration + 'px'}" 
    >
  <h5>{{ event.title }}</h5>
</div>

I would recommend in the future to use a Room component and a RoomEvent component so that you could write:

<Room v-for="room in rooms" :key="room.id" />

And the room-component:

<div class="room">
  <span>{{ room.title }}</span>
  <RoomEvent v-for="event in room.events" />
</div>

I think that the <h5>{{ event.title }}</h5> belongs inside the RoomEvent


Overall a nice job!

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for such a great review! I rewrote backend as I mentioned. Now the code is a bit faster. As for CSS, I will look at CSS Grids one more time, but I'm afraid it's more complicated and less supportive by old browsers way. Anyway, thanks for your advice! \$\endgroup\$ – Alex Mar 7 at 7:22
  • \$\begingroup\$ Another small question: I have some plugins that need jQuery. Is it a good idea to use both jQuery (UI components like Select2) and Vue in same code? \$\endgroup\$ – Alex Mar 7 at 7:24
  • \$\begingroup\$ @Alex Yes unfortunately CSS Grids are not supported by older browsers. Regarding jQuery: I have never used it together with Vue myself but I would recommend to try to get rid of jQuery. There is probably Vue alternatives that you can use instead, if you are looking for UI-components then I can recommend Vuetify for example. \$\endgroup\$ – Simon Forsberg Mar 7 at 12:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.