2
\$\begingroup\$

I made a basic to-do list in VueJS, but it feels really clunky and obtuse to me. One of the key things is that if the text input value is empty and the user clicks the button labeled submit then it shows a red error. When you start typing, it goes away. I tried declaring the variables in a different scope but it breaks the application. See full sample below:

// components
Vue.component('todoitem', {
  template: "<li>Test Item</li>"
})

// app code
var app = new Vue({
  el: '#app',
  data: {
    todos: [],
    button: {
      text: 'Hide'
    },
    seen: true
  },
  methods: {
    addItem: function() {
      let item = document.getElementById("list-input").value;
      let error = document.getElementById("error");
      if (item == "") {
        error.style.display = "block";
      } else {
        app.todos.push({ text: item });
        error.style.display = "none";
      }
    },
    removeItem: function(index) {
      this.todos.splice(index, 1);
    },
    removeError: function() {
    	let item = document.getElementById("list-input").value;
      let error = document.getElementById("error");
    	if (item == "") {
        error.style.display = "block";
      } else {
        error.style.display = "none";
      }
    },
    toggleSeen: function() {
      app.seen = !app.seen;
      app.button.text = app.seen ? 'Hide' : 'Show';
    }
  }
});
#app {
  width: 900px;
  padding: 2em;
  margin: 0 auto;
}

.todo-list {
  list-style-type: square;
}

.todo-list__delete {
  display: none;
}

li:hover .todo-list__delete {
  display: inline-block;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.5.3/vue.min.js"></script>
<div id="app">
  <h3>Add an Item to the list</h3>
  <ul class="todo-list">
    <li v-for="(todo, index) in todos">
      {{ todo.text }}
      <a v-on:click="removeItem(index)" class="todo-list__delete" href="#">Delete</a>
    </li>
  </ul>

  <input v-on:keyup="removeError" type="text" id="list-input">
  <input type="submit" id="list-submit" v-on:click="addItem">
  <span id="error" style="color: red; display: none;">Please Enter Text</span>

  <ul>
    <todoitem></todoitem>
  </ul>

  <h2 v-if="seen">SEEN</h2>
  <button id="hide-seen" v-on:click="toggleSeen">{{ button.text }}</button>
</div>

How can I make this better?

\$\endgroup\$
2
\$\begingroup\$

DOM references

If you are using functions to lookup DOM elements (e.g. document.getElementById()) for elements contained within the root element (or component), it might likely be the wrong way of doing things, as far as VueJS code goes. With VueJS, you can use v-model to bind the value of the input to a data property - for example, the <input> for the new list item can have a v-model attribute added, like:

<input v-on:keyup="removeError" type="text" id="list-input" v-model="newItem">

And it would be wise to add an entry in the data object for that model value:

data: {
    todos: [],
    //...
    newItem: ''

Then when creating an item, use this.newItem for the value.

Conditional Rendering

The error message can be conditionally rendered using v-if (or v-show, depending on how the DOM should be laid out in various cases). Read more about conditional rendering here.

That way there is no need to lookup the DOM element and set the style to display: block, display: none, etc.

Method context

In the method toggleSeen() the variable app is used, but realize that the method is called in the context of app so this can be used instead (of app).

Computed properties

Use computed properties to control values that depend on other values. For example, the button text can be added to a computed property. Remove button from data and then add a computed property:

computed: { button: function() { return { text: this.seen ? 'Hide' : 'Show' }; } }

That way the logic for the text property can be removed from the method toggleSeen(). Then that method call could be replaced by an inline expression, or else a checkbox could be used with the v-model attribute bound to seen.

// components
Vue.component('todoitem', {
  template: "<li>Test Item</li>"
})

// app code
var app = new Vue({
  el: '#app',
  data: {
    todos: [],
    seen: true,
    showError: false,
    newItem: '',
  },
  computed: {
    button: function() {
      return {
        text: this.seen ? 'Hide' : 'Show'
      };
    }
  },
  methods: {
    addItem: function() {
      this.showError = (this.newItem == "");
      if (!this.showError) {
        app.todos.push({
          text: this.newItem
        });
        this.newItem = '';
      }
    },
    removeItem: function(index) {
      this.todos.splice(index, 1);
    },
    removeError: function() {
      this.showError = false;
    },
    toggleSeen: function() {
      this.seen = !this.seen;
    }
  }
});
#app {
  width: 900px;
  padding: 2em;
  margin: 0 auto;
}

.todo-list {
  list-style-type: square;
}

.todo-list__delete {
  display: none;
}

li:hover .todo-list__delete {
  display: inline-block;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.5.3/vue.min.js"></script>
<div id="app">
  <h3>Add an Item to the list</h3>
  <ul class="todo-list">
    <li v-for="(todo, index) in todos">
      {{ todo.text }}
      <a v-on:click="removeItem(index)" class="todo-list__delete" href="#">Delete</a>
    </li>
  </ul>

  <input v-on:keyup="removeError" type="text" id="list-input" v-model="newItem">
  <input type="submit" id="list-submit" v-on:click="addItem">
  <span v-if="showError" id="error" style="color: red;">Please Enter Text</span>

  <ul>
    <todoitem></todoitem>
  </ul>

  <h2 v-if="seen">SEEN</h2>
  <button id="hide-seen" v-on:click="toggleSeen">{{ button.text }}</button>
</div>

\$\endgroup\$
  • \$\begingroup\$ Very helpful. Thank you for taking the time to review. \$\endgroup\$ – kawnah Nov 10 '17 at 14:31

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.