meteor / todos

The example app "Todos", written following the Meteor Guide
Other
535 stars 365 forks source link

Can't edit a list #106

Closed HugoCornu closed 8 years ago

HugoCornu commented 8 years ago

It seems I am unable to edit a list name. (using Chrome)

Clicking the list name doesnt seems to do anything But clicking the the list name then the new todo field then the list name again, triggers an alert : "validation -error"

tmeasday commented 8 years ago

Repro-ed

electr0sheep commented 8 years ago

I was actually working on adding this functionality. I thought that it was by design. I attempted to allow a new list to be created in the same fashion as a new item in a list. However, I believe that you cannot simply create a list and call it whatever you want for security purposes (one of the functions seems to do the same thing every time i.e. it cannot have parameters. I was able to name a new list by calling Date(), but I could not pass a parameter to this function to name it something specific). For reference, 'click .js-new-list'() in imports/ui/layouts/app-body.js calls insert from imports/api/lists/methods.js (This method is a ValidatedMethod), which in turn calls insert from the class ListsCollection in imports/api/lists/lists.js.

It didn't occur to me to edit the name after the list was created. Upon inspection, it appears that the following segment of code is supposed to be responsible for allowing a user to click on the list at the top of the screen an edit it's name. From todos/imports/ui/components/lists-show.js:

this.editList = () => {
  this.state.set('editing', true);

  // force the template to redraw based on the reactive change
  Tracker.flush();
  // TODO -- I think velocity introduces a timeout before actually setting opacity on the
  //   element, so I can't focus it for a moment.
  Meteor.defer(() => {
    this.$('.js-edit-form input[type=text]').focus();
  });
};
petestreet commented 8 years ago

@electr0sheep When I was trying to track down the source of the error, I found that the the actual problem only occurs when you move from the editing state which you've posted above, and then clicking anywhere else on the page. The 'blur input[type=text]' event calls saveList(), and since the form and input for the list's name wouldn't render (instance.state.get isn't automatically available in the template, as far as I could tell), the value of the input is null and saveList() tries to save an empty title. This is what I tried to fix in https://github.com/meteor/todos/pull/112

As for the design concern of arbitrary list names, I'd argue that sanitizing names ought to be done inside the appropriate ValidatedMethod — else why would we provide the 'editing' state and associated events at all?

(also, sorry to butt in out of the blue. I've been going through the todos example to understand Flow and 1.3 better, so there's selfish motivation for me to contribute here.)