PlastronJS #163

Closed
wants to merge 17 commits into
from

3 participants

@rhysbrettbowen

added in architecture example for plastronJS https://github.com/rhysbrettbowen/PlastronJS.

includes the compiled and uncompiled versions of the code.

@sindresorhus
TasteJS member

Thanks :)

Make sure you've read our Contribution Guidelines and our App Spec.

  • New apps should be placed in the labs/architecture-examples/ folder.

  • You're including a lot of seemingly unnecessary files. Can you only include a concatenated and minified version of the Closure Library?

  • Names of folder should be singular: lib, test, ...

  • There is a lot more, please read the spec thoroughly. Let me know if you have any questions.

@gmkumar2005

rhysbrettbowen Thanks for the lovely framework. Thanks for the example code. I found few issues with the architecture example. 1) The text input box does not clear itself. After i press enter key the text is added to the todo list but the value in the text box remains as is. 2) More than one items remain in editable mode. ie, if I double click each item each item renders as editable but all the items previously clicked also remain editable. Expected behavior is - only one item should be in editable mode. 3) The clear completed widget does not display count.

@rhysbrettbowen

okay, hopefully these are all the changes that need to happen. I've moved it to the labs folder and changed the template and css (copied them from another labs project and edited). Also went through and made sure all the functionality was in there.

Haven't got IE with me on this computer so haven't been able to test that (the blur event is delegated so the might not work).

Also changed it to use plovr - I saw that the closure example used that so removed the closure-library lib and put that instead.

let me know if I've missed anything

@sindresorhus sindresorhus commented on an outdated diff May 7, 2012
...ure-examples/plastronjs/js/controllers/todocontrol.js
+};
+goog.inherits(todomvc.todocontrol, mvc.Control);
+
+
+
+/**
+ * overrides goog.ui.Component#createDom with the todo template.
+ *
+ * @inheritDoc
+ */
+todomvc.todocontrol.prototype.createDom = function() {
+ var el = goog.dom.htmlToDocumentFragment('<li>' +
+ '<div class="view">' +
+ '<input class="toggle" type="checkbox">' +
+ '<label>' + this.getModel().get('text') + '</label>' +
+ '<a class="destroy"></a>' +
@sindresorhus
TasteJS member
sindresorhus added a line comment May 7, 2012

Should be <button>, see template.

Also, does PlastronJS have any templating?
String concatenating is plain bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff May 7, 2012
...ure-examples/plastronjs/js/controllers/todocontrol.js
+ // delete the model
+ this.click(function(e) {
+ e.preventDefault();
+ this.getModel().dispose();
+ return false;
+ }, 'destroy');
+
+ // dblclick to edit
+ this.on(goog.events.EventType.DBLCLICK, function(e) {
+ this.getParent().makeChildEditable(this);
+ }, 'view');
+
+ // save on edit
+ var inputEl = this.getEls('.edit')[0];
+ this.on(goog.events.EventType.KEYUP, function(e) {
+ if (e.keyCode == goog.events.KeyCodes.ENTER &&
@sindresorhus
TasteJS member
sindresorhus added a line comment May 7, 2012

use === everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff May 7, 2012
...ure-examples/plastronjs/js/controllers/listcontrol.js
+ * setup for event listeners.
+ *
+ * @inheritDoc
+ */
+todomvc.listcontrol.prototype.enterDocument = function() {
+ goog.base(this, 'enterDocument');
+
+ var list = /** @type {Object} */(this.getModel());
+
+ // create new model from text box
+ var input = this.getEls('input')[0];
+ this.on('keyup', function(e) {
+ e.preventDefault();
+
+ // on return get trimmed text
+ if (e.keyCode != goog.events.KeyCodes.ENTER) return;
@sindresorhus
TasteJS member
sindresorhus added a line comment May 7, 2012

!==

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff May 7, 2012
labs/architecture-examples/plastronjs/js/app.js
+var todolistControl = new todomvc.listcontrol(todolist);
+todolistControl.decorate(goog.dom.getElement('todoapp'));
+
+//setup router
+var router = new mvc.Router();
+
+
+/**
+ * @param {number} index of selected filter.
+ */
+var toggleFilters = function(index) {
+ var filters = goog.dom.getElementsByTagNameAndClass('A', undefined,
+ goog.dom.getElement('filters'));
+ goog.dom.classes.enable(filters[0], 'selected', index == 0);
+ goog.dom.classes.enable(filters[1], 'selected', index == 1);
+ goog.dom.classes.enable(filters[2], 'selected', index == 2);
@sindresorhus
TasteJS member
sindresorhus added a line comment May 7, 2012

Would be better to compare names rather than index. What if I move around the links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus
TasteJS member

Thanks, much better 😃

I've added some inline comments and some here:

  • The items word should be item when there are only one item.

  • Don't include the base.css and bg.png, but reference them from the assets folder.

  • Can you copy the README from the template. And add your name and a description of the framework? Also with any instruction on how to use if nessecary.

  • There are still a lot of uneeded files. Can you concat and minify the PlastronJS lib too? This goes for any other included library too. You don't need to include library docs either. We have a lot of frameworks now. And our repo is getting big. So we're trying to keep it down as much as we can. Hope you can understand.

  • The todo content in localStorage should be names title. See app spec.

@rhysbrettbowen rhysbrettbowen keep only necessary lib files
add README
change text to title
change == & != to === and !==
put in templates
da19693
@rhysbrettbowen

okay - hope this does it.

I've used the closure-templates (soy) for the HTML generation which is what I'd usually recommend.

I've removed all the files from PlastronJS that I can (all the docs, tests and a couple of the js files) - I can't minify the code as it's designed to be included with the project when using the closure compiler.

also made the other changes you've recommended and put in a README.md

@gmkumar2005

1) When item is marked as complete the font is not changing to strike-out
2) When "mark all as complete" is selected the font of the items is not changing to strik-out
3) The items marked as complete disappear from the list.
To simulate -- a) create few items b) switch to active route c) click the complete checkbox(the tick mark).
The item disappears from the list. Expected behaviour is the font of the item should change to strike-out and remain in the view.
4) Same as above but for "completed". Expected behaviour is Expected behaviour is the font of the item should change from strike-put to normal
and remain in the view
5) Same as above but when the completed router is selected. When i add an item it does not appear. Expected behaviour is, the new item
is displayed in the list without strike-out

Above is tested in Google Chrome 18.0.1025.162 on Linux OS

6) With firefox ver 11, the functinality is same as above. The styling has one issues
a) the checkboxes appear as regular square bracket. expeted behaviour is tickmark image

@gmkumar2005

I am not able to find the soy ( google templates) files

@rhysbrettbowen

sorry - my fault. forgot to add in the new files

@rhysbrettbowen

for 3 & 5 - shouldn't they disappear? the filter is to only show active so if they're changed to completed they should no longer be shown (as they're no longer active). I can change this no worries though.

sounds like I'm not putting the class on properly for completed so I'll take a look and get back to you

@rhysbrettbowen

had a quick look and added the strike through to an app.css - should this be handled in the base.css in assets?

@rhysbrettbowen

also for the check mark, looks like it could be a stylesheet issue. I'll look through the other labs/ projects and see if they've got the same issues

@rhysbrettbowen

http://www.w3.org/TR/2005/WD-CSS21-20050613/generate.html#before-after-content

looks like the checkboxes are what the spec says so the base.css in the assets folder isn't correct. The :before and :after pseudo elements shouldn't work on elements with no DOM subtree (input, br, img etc.) so firefox is correct.

Can you let me know how the filters should handle? It's a bit strange if the user has on the active filter and they mark something complete and it's still being shown under the active filter. Perhaps the filter should be turned off after an interaction? I had a look at the normal projects on the page and they don't seem to have routing enabled - though the stapes one in labs just returns the user to the "all" view after any interaction - should I do that?

@rhysbrettbowen

also just noticed that the todomvc labs link on http://addyosmani.github.com/todomvc/ points to http://todomvc.com/labs instead of "labs/" so you might want to fix that

@rhysbrettbowen

because I'm using refresh to re-render all the children I've cleaned up the code and removed unnecessary updates and put a bit of logic in the templates. Should make the example cleaner and easier to understand.

@gmkumar2005

for 3 & 5 i am not sure abt the expected behavior. The app spec does not say anything. I have commented based on the behavior in spine.js example. When items disappear it ls confusing to the user. I prefer the behaviors as in the spine.js.

@gmkumar2005

Rhys ,
Staples implementation seems incomplete. The edit control is in wrong shape and position, After an action while in active route, it automatically switch to ALL, but you cannot go back to active, unless u click ALL or Completed. All edit controls open up . The ALL, Active,Complete does't switch to bold when selected.
I like the idea of switching to ALL route - but prefer the behavior in SpineJS
Sindre Sorhus What do u think ?

Issue 1,2 is resolved. 3,4,5 and 6 not resolved.
Interesting side effect of 3,4,5 is with active route selected, when i toggle the "mark all as complete" all the records suddenly disappear and appear again when i toggle again. SpineJS has different behavior.

The items are made invisible after you switch from current route and then back.

I tried on ie8 the situation is hopeless. Same with most of the other examples. each example has a different behavior. none have full functions working on ie8.

@sindresorhus
TasteJS member

@rhysbrettbowen

See my inline comments.

  • Why can't you concat and minify the PlastronJS lib?

for 3 & 5 - shouldn't they disappear? the filter is to only show active so if they're changed to completed they should no longer be shown (as they're no longer active). I can change this no worries though.

  • 3-5 is not correct. Your current behavior is.

also for the check mark, looks like it could be a stylesheet issue. I'll look through the other labs/ projects and see if they've got the same issues

  • I know it's an issue. When I designed the template, I didn't remember input elements could not have :before/:after. Will change this at some point, but right now it works correctly even though it looks weird different in some browsers.

  • Also make sure to update the compiled js.

@gmkumar2005

Thanks for your help :)

It's the spine.js example that is wrong. I'll update the app and spec.

About IE8. I'll fix some IE8 problems before the 1.0 release. Just haven't had the time to do it yet.

@rhysbrettbowen rhysbrettbowen change done to complete class
remove plastronjs and css
add instructions in README
7b129a5
@rhysbrettbowen

okay, I've removed plastronjs and plovr and instead put instructions how to build the project in the README.

The only really necessary file in the compiled.js, the rest of the js doesn't run butis necessary so people can actually understand the code, and the plovr.json is there so they can build the project.

I've changed the class to completed.

For the other things - PlastronJS is not designed to be minified on it's own, the purpose is that it gets minified together with the rest of the project for greatest efficiency. If you have a look at the example you'll see there is only one compiled.js and no other js file being brought in. I've removed the whole library though and just pointed users to the download and told them to put it in the file if they want to play around with it.

it has to use bracket notation because of the compiler. The compiler will rename things that use dot notation and as we want it to save as 'title' in localStorage it has to be called with bracket notation throughout the project to keep that intact. The basic rule of thumb for the closure compiler is that if a value exists externally then use bracket notation, if it's internal only (and doesn't need to match a string) then use dot.

I've removed the app.css now I've got the right class - had "done" from when I was looking at the old template sorry

the sync.js acts as an interface. It's used in the compiler's type detection and just gets thrown away when it's compiled. It was there to stop the compiler from giving warnings as it wouldn't be able to resolve the localSync as an mvc.Sync type without the file (as the mvc.Sync type wouldn't have been defined).

updated the compiled.js

think we're there now? thanks for all the help getting this up to scratch.

@gmkumar2005

1,2 --resolved closed, 3,4,5 clarified and closed
6) With firefox ver 11, the functinality is same as above. The styling has below issues
a) the checkboxes appear as regular square bracket. expeted behaviour is tickmark image -- OPEN
b) The text overflows when looong word is entered. text wraps when chrome browser is used

@gmkumar2005

I have tested plastronJS and SpineJS for XSS using vectors in http://ha.ckers.org/xss.html
All tests passed - Chrome browser is tested. FF not tested IE not tested.
Testing process :- Copy the vector from above link, paste into the input field in todomvc application, press enter
Success criteria :- Popup/alert should not occur, The page should not get distorted.
Result :- All vectors passed on chrome browser 18.0.1025.162 , linux OS

@sindresorhus sindresorhus added a commit that closed this pull request May 10, 2012
@rhysbrettbowen rhysbrettbowen Close #163: PlastronJS app 9e536a3
@sindresorhus
TasteJS member

@rhysbrettbowen Landed. Thanks!

@gmkumar2005 6. I know about it, but it's the template, and has nothing to do with this app.

@gmkumar2005

Unable to compile..
Seems there is an issue with pull or commit.
When i download the code from https://github.com/rhysbrettbowen/todomvc
i am able to compile using plovr and the app works perfectly.
But when i download from https://github.com/addyosmani/todomvc app works but the with plovr it behaves weirdly. I tried even the raw option.
I compared both the /plastron folders. I fond very simple changes like formatting and small case comments converted into proper case. Not able to figure out what is happening ...

@rhysbrettbowen

build seems to work fine for me:

Rhyss-MacBook-Air:tuts rhysbrettbowen$ git clone https://github.com/addyosmani/todomvc.git
Cloning into todomvc...
remote: Counting objects: 4447, done.
remote: Compressing objects: 100% (2470/2470), done.
remote: Total 4447 (delta 1837), reused 4138 (delta 1657)
Receiving objects: 100% (4447/4447), 17.99 MiB | 346 KiB/s, done.
Resolving deltas: 100% (1837/1837), done.
Rhyss-MacBook-Air:tuts rhysbrettbowen$ cd todomvc/labs/architecture-examples/plastronjs/
Rhyss-MacBook-Air:plastronjs rhysbrettbowen$ mkdir build
Rhyss-MacBook-Air:plastronjs rhysbrettbowen$ cd build/
Rhyss-MacBook-Air:build rhysbrettbowen$ curl -C - -O http://plovr.googlecode.com/files/plovr-4b3caf2b7d84.jar
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 10.2M 100 10.2M 0 0 538k 0 0:00:19 0:00:19 --:--:-- 562k
Rhyss-MacBook-Air:build rhysbrettbowen$ mv plovr-4b3caf2b7d84.jar plovr.jar
Rhyss-MacBook-Air:build rhysbrettbowen$ cd ../js/
Rhyss-MacBook-Air:js rhysbrettbowen$ mkdir lib
Rhyss-MacBook-Air:js rhysbrettbowen$ cd lib
Rhyss-MacBook-Air:lib rhysbrettbowen$ git clone git://github.com/rhysbrettbowen/PlastronJS.git
Cloning into PlastronJS...
remote: Counting objects: 296, done.
remote: Compressing objects: 100% (177/177), done.
remote: Total 296 (delta 157), reused 252 (delta 113)
Receiving objects: 100% (296/296), 9.67 MiB | 380 KiB/s, done.
Resolving deltas: 100% (157/157), done.
Rhyss-MacBook-Air:lib rhysbrettbowen$ cd ../..
Rhyss-MacBook-Air:plastronjs rhysbrettbowen$ java -jar build/plovr.jar build plovr.json
0 error(s), 0 warning(s), 95.98% typed
Rhyss-MacBook-Air:plastronjs rhysbrettbowen$

what are you seeing with plovr?

@gmkumar2005

I did not use git clone? Downloaded the code as zip.
Compiling ends with success . The compiled code does not run. When I open the application without compiling it runs correctly.
When I compile the code using plovr the freshly compiled code does not run in the browser. The compiler output is same as what u got above. In case ur not able to simulate let me know, I will try again and send the logs

@rhysbrettbowen

I took a video showing downloading the todomvc examples here:
http://modernjavascript.blogspot.com/2012/05/todomvc-plastronjs-example.html

Are you changing the js file src at the bottom of index.html?

@gmkumar2005

I have seen the video it is very good. I think the resolution can be improved . I was not able to see the characters.
In the video u have demonstrated how to compile. My issue is after compiling the application does not work. It works as extected when i download from ://github.com/rhysbrettbowen/todomvc . I will try to simulate the exact steps as in the video and post the result on Tuesday

@rhysbrettbowen

thanks for that, I've added a lint at the bottom to direct users to click the youtube button and then view in fullscreen if they're having trouble reading the characters.

Are there any parts that are particularly hard to read? I can put annotations up for those bits

@rhysbrettbowen

I've found the issue - was going through and playing with it.

The problem is that all the spaces were changed to tabs and this caused a problem with the template. The template has a \t character and then the li element so when it is creating the template it is instead returning a document fragment instead of the li element.

@sindresorhus - can you remove the tabs from template/templates.soy and change them back to spaces? or if you just get rid of the tab before the li (just un-indent everything by one) that should fix it.

@rhysbrettbowen

I've updated my repo to take away the whitespace in front of the li element

I've also refactored the todocontrol (inlined the makeEditable function and removed the ability to make uneditable as it's not needed).

Should I change my code to use tabs and have spaces in the brackets? I didn't have these originally as the code is meant to be inline with closure linter

whats the best way to get these changes in? should I open a new pull request?

@sindresorhus
TasteJS member

Sure, submit a pull, just make sure to rebase upon our master.

I did some style changes after I pulled in your last pull. I'm sorry I didn't check it afterwards, but since it was only style changes I was confident it wouldn't break anything.

As you can see in our App Spec we require tabs.

@gustaff-weldon gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013
@rhysbrettbowen rhysbrettbowen Close #163: PlastronJS app 90e0c4c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment