search

grg021 / pfflipper

Page Forest APP - Flipper: A split-flap display simulator
5 stars 1 forks source link

Code review issues #33

Open mckoss opened 13 years ago

mckoss commented 13 years ago

I just took a look at your code in more detail, and have run into quite a few questions.

First, I wanted to have a 900px with set as the width of the body. When I did so, all the letter sizes are wrong. So I was confused about why your code does not pick up the size from the page's css.

See http://flipper.pageforest.com for my version (also at http://github.com/mckoss/pfflipper).

When I look at onReady, I see that you are resetting the widths of several elements in code. Why not just style them with css, and have them be flexible on window size change as well.

I'm also confused about the calls to (jQuery)(fn) in your code. Isn't this just another way to run code at the same time as onReady? But when defined this way, I'm not sure you can count on the relative ordering of the 3 function calls (and you do require that one is called before the other).

Also, there should be NO calls to $ajax in your code. You just need getDoc and setDoc functions. Yet you seem to have some explicit GET, PUT, and LIST functions here. Why is that?

Why did you turn auto-save off? If you do that, you need to call setDirty() manually. I'd prefer you didn't turn it off for this example.

mckoss commented 13 years ago

BTW - there is some more written docs about the pageforest client api here:

http://wiki.pageforest.com/#pageforest-api/client

I know when you started the docs were more minimal so it looks like you copies some code from other examples?

mckoss commented 13 years ago

Gahr! Accidentially closed an issue again!!!

grg021 commented 13 years ago

Switch to #main.width() instead of window.width() to pickup the "width" from CSS if defined.

grg021 commented 13 years ago

RE: jquery onReady: Cleaned up the code. Incorporated the other functions to the onReady function.

RE: $.ajax calls: ajax - get, list -> to get a unique ID for the document based on the number of existing documents he/she has. ajax - put -> to save the document and redirect to display.html when finished saving.

Should I just use a random UUID generator instead, for the docID and the "link to display" button will only save the document and provide a link through a modal box?

mckoss commented 13 years ago

You actually only need setDoc() and getDoc() - all the ajax calls are handled by the Pageforest library.

My repo has a version with these functions stripped out.

grg021 commented 13 years ago

Ok, I've merged with repo, thanks!