rappidjs update

[krebbl]

Updated app again.

There are 2 issues open:

• There is no local storage
• the folder structure is different from yours and can't be changed
  On issue 1 we are working, but it could take some time, because there are other things to to at the moment (giving presentations and so on). Nonetheless would it be possible to accept the pull request? We would like to see our app public.

Thank you
Regards

Followup to #140

[krebbl]

Is this pull request ok?

[sindresorhus]

Almost, as I said in the other thread, it needs to be complete before merging it in, which means localStorage needs to be implemented first ;)

[krebbl]

yes, I just meant the pull request :D

locale storage is on it's way!

[sindresorhus]

@krebbl Any updates? We're nearing the 1.0 release, and need to know if you'll be able to get it done before the release, otherwise we can defer it to the next release ;)

[krebbl]

We are actually working on the DataSource Adapters. Till when do you plan to release 1.0?

[sindresorhus]

We're planning to release it on june 20th. Will you be able to get it done by then?

[krebbl]

ok perfect, we will get this done by then.

[krebbl]

Hey,

just updated the pull request. I hope everything works.

And could you please correct the framework name and link to the project page on the labs page. It is called "rAppid.js" and our project page can be found under www.rappidjs.com.

Thanks

[sindresorhus]

Not needed, included in ie.js. See template.

[sindresorhus]

@krebbl Please see @addyosmani comments in your previous pull #185. You still have your app in both / and /labs.

Some comments:

• When I check a todo it doesn't get a strike through
• Editing mode should be activated by double-click on .view, not the label:

Double-clicking the container (.view) activates editing mode,

• You're saving the UUID of the todo item twice in localStorage. Is there a reason?
• Use reference base.css in the assets folder. Remove app.css.
• Please see the template HTML, your app does not follow it.

[krebbl]

Thanks for your review, we fixed the mentioned points. One more thing. Since there is no content in the body, we don't put the scripts at the end of the body. So we don't need to listen for the DOM ready event.

You're saving the UUID of the todo item twice in localStorage. Is there a reason?

We are saving also a collection of the todos with the UUID as reference key. This collection is accessed when we call todoList.fetch(). I think this is the correct implementation of a key value storage.

[krebbl]

by the way, should I remove the folder in architecture examples for the pull-request?

[sindresorhus]

Will review later.

Yes please, on the removing of folder.

[sindresorhus]

We are saving also a collection of the todos with the UUID as reference key. This collection is accessed when we call todoList.fetch(). I think this is the correct implementation of a key value storage.

Thanks for explaining. I don't think there are any right or wrong way to do it. I usually use an array to be able to keep the order. The obvious upside with that is that you automatically have an index and doesn't need to save the UUID in two places.

Some comments:

• The CSS path is wrong. Please test before submitting ;)
• Remove console.log
• "All" filter should be selected by default.
• Filters should get the "selected" class when active.
• ⬇️ ?

Can you consider changing the license of the todo app to Public Domain? We're trying to keep everything under the same license.

[krebbl]

The obvious upside with that is that you automatically have an index and doesn't need to save the UUID in two places.

Yes, one place sounds better. :) And to save it as an array is a good idea to keep the order. But I'm asking me how do you fetch a single item by it's UUID? It's not necessary for this application, but with your approach you would have go through the items till you find the correct one? I think for reference reasons it's better to have one or more separate indices.

The CSS path is wrong. Please test before submitting ;)

I linked the asset folder in the wrong directory. ../../assets/base.css is right?

Remove console.log

Will remove

"All" filter should be selected by default.
Filters should get the "selected" class when active.

"All" filter is selected, but with wrong class. Will fix this

And I will change the License.

[sindresorhus]

But I'm asking me how do you fetch a single item by it's UUID? It's not necessary for this application, but with your approach you would have go through the items till you find the correct one? I think for reference reasons it's better to have one or more separate indices.

Both methods work fine. Preference I guess.

I linked the asset folder in the wrong directory. ../../assets/base.css is right?

You could always just check yourself? It's ../../.. i think

Thanks :)

[sindresorhus]

Ok, a few more comments and I'm done. Promise!

• Can you remove the leftovers of the localization stuff. Like the locale folder, the i18 string stuff in the code, etc.
• I see you got some JS code in TodoView.xml. Any chance to extract that JS and instead reference it? Inline JS is dirty, especially when inlined in XML.

[krebbl]

Can you remove the leftovers of the localization stuff

The i18n string stuff is also used for pluralization (see "items" and "item"), but I can put this also in an extra function. Is there already a feature request for i18n support in the todomvc app? I think this would be nice, since it is an important part of todays applications.

Any chance to extract that JS and instead reference it?

Yes this is possible, but it's also a feature to use script in the X(A)ML file. We wanted to present it that way. Other frameworks use code in their templates. I think thats really dirty. If you don't accept it, we will put it in a code behind file.

Thanks for review so far!

[sindresorhus]

The i18n string stuff is also used for pluralization (see "items" and "item"), but I can put this also in an extra function. Is there already a feature request for i18n support in the todomvc app? I think this would be nice, since it is an important part of todays applications.

That would be nice.

There currently is not, but feel free to file a ticket outlining why you feel it should be a feature ;)

Yes this is possible, but it's also a feature to use script in the X(A)ML file. We wanted to present it that way. Other frameworks use code in their templates. I think thats really dirty. If you don't accept it, we will put it in a code behind file.

Ok, sure, keep it as is.

[sindresorhus]

@krebbl Clicking the filters at the bottom now doesn't do anything, regression.

[krebbl]

so, I fixed the filters and we changed a little bit the event handling ... I added also event propagation stopping for the double click on the checkbox, so that the editing mode don't get's enabled ...

[sindresorhus]

@krebbl Merged. Thanks for this addition :)

[krebbl]

@sindresorhus I'm happy to hear. :)

One comment from our side:
Could you please change the framework name to rAppid.js and the homepage link to www.rappidjs.com on your site?

Thank you

[sindresorhus]

Yup, I'm afk atm, but it's already on my todo list ;)