Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GWT implementation #103

Closed
wants to merge 19 commits into from
Closed

Added GWT implementation #103

wants to merge 19 commits into from

Conversation

ColinEberhardt
Copy link
Member

Hi,

I have created a GWT implementation of the TodoMVC application, following your latest specification / template. I know that this version is a little different to the others (and not just because it follows the MVP pattern rather than MVC!).

I have included the compiled code which allows you to run the application, together with the src folder, which includes the Java source.

I think this example could be useful for people show are evaluating the difference in approach between programming in JavaScript directly and using Java to compile to JavaScript.

Regards, Colin E.

@addyosmani
Copy link
Member

Hey @ColinEberhardt. First, thank you very much for taking the time out to put this together.

I'd love to include a GWT example in the project and it's fine that it's MVP rather than MVC (most of our apps are using a variation of MV*).

We'll review this for inclusion in our next major release (1.0).

Cheers!
Addy

Whilst this application is very different to the other TodoMVC implementations, it still makes for
an interesting comparison. Large-scale JavaScript applications are often written with GWT or Closure,
with the resulting JavaScript code delivered to the client being compiled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonus points for including a readme about the implementation :bowtie:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a section on how to compile it?

Also add which GWT and Java version used.

@sindresorhus
Copy link
Member

Thanks for the contribution, we really appreciate it 😃

I've reviewed the commit and added some inline comments. Some more:

  • Why does it use an iframe? Any way around that? It doesn't work locally on Chrome because of security restrictions. No problem starting a local webserver, but not everyone knows how to do that.
  • Could you use the same folder structure and filenames as used in the template? The Java source can stay in the src folder. The cached html can stay in a folder named cache and the .js can be moved to the js folder.
  • Could you use the app.css from the template folder? I see some differences with yours and the template one. If you have to change something, create a separate override.css file and put the overriding rules in that, but try to minimize overriding rules as much as possible. Since the amount of todo examples has grown so large, we need to have a single unified css file, in case of changes.
  • You can remove the destroy.png. It's embedded in the CSS file.
  • Can you prevent the generation of the *cache.png images? Since they're not used. If not, at least remove them from the commit.
  • What's the purpose of the cache .html files? They're quite big.

<section class="main" ui:field="mainSection">

<g:CheckBox ui:field="toggleAllCheckBox"></g:CheckBox>
<label for="toggle-all">Mark all as complete</label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkbox need to have the id toggle-all to make the label clickable.

@ColinEberhardt
Copy link
Member Author

Hi @sindresorhus - many thanks for the thorough review comments. I should be able to resolve most of these tomorrow.

The only one that could be trick is 'restoring' the ul / li semantics. The GWT implementation uses a CellList - a framework widget for rendering a list of cells. Unfortunately the CellList uses a div as its root container and wraps each cell in a separate div. Unfortunately there are no extension points that allow you to change this.

I could create my own cell renderer to correct this, however, it would involve quite a bit of code.

I think this issue highlights a key difference between GWT and the other JavaScript frameworks. With GWT you rarely try to precisely match a given HTML markup.

Anyhow, I plan to put this all in a blog post in the near future. I will link to it from the readme file.

Colin E.

@ColinEberhardt
Copy link
Member Author

@sindresorhus I'll look into cleaning up some of the generated output, just FYI, the HTML files contain the compiled JavaScript for this application. There are 6 versions because GWT works with a concept called 'permutations', separate versions of the application are created for each permutation. With this application there are 6 permutations each targeting a different browser - however in a production app you might have permutations to target different languages / locales.

This means the code delivered to the browser is specific to the browser version being used.

The files are large because of the general GWT overhead. It isn't meant to be lightweight, being intended for use on large scale applications.

Colin E.

@sindresorhus
Copy link
Member

The only one that could be trick is 'restoring' the ul / li semantics. The GWT implementation uses a CellList - a framework widget for rendering a list of cells. Unfortunately the CellList uses a div as its root container and wraps each cell in a separate div. Unfortunately there are no extension points that allow you to change this.

@ColinEberhardt Just though we could minimize the need for CSS overrides, but this sounds like too much extra work to be worth it, not that important. I tried searching the GWT mailing-list and Google, and I'm amazed that no one has had the same question.

I think this issue highlights a key difference between GWT and the other JavaScript frameworks. With GWT you rarely try to precisely match a given HTML markup.

Yeah it really does... I've noticed GWT has some superbly bad HTML output.

Anyhow, I plan to put this all in a blog post in the near future. I will link to it from the readme file.

Cool, let us know when it's published ;)

I'll look into cleaning up some of the generated output, just FYI, the HTML files contain the compiled JavaScript for this application. There are 6 versions because GWT works with a concept called 'permutations', separate versions of the application are created for each permutation. With this application there are 6 permutations each targeting a different browser - however in a production app you might have permutations to target different languages / locales.

Wait, what!?! This sounds like something we did in the late nineties... How does it work? Do you have to recompile when there's a new browsers released? And what about browser versions?
Let's see: browers * versions * languages * other factors = a lot of permutations :P

@sindresorhus
Copy link
Member

Found another thing, there should be a space between the checkbox and the label, in the listItem. I have no idea why there isn't one, since it should be there by default.

@ColinEberhardt
Copy link
Member Author

(not sure how you reply with a block quote here)

With reference to the 'permutations', I should have been more specific. GWT detects user-agent, then has permutations that target specific 'problem' cases, for example there is a permutation for ie6, but not ie7, ie8, ie9. Therefore, future compatibility is most likely not going to be an issue.

This isn't so different from the thoroughly modern approach of using JavaScript to write your CSS because of all the repetition needed in order to output various prefixes that are currently required!

On the plus side, GWT ensures that the 'junk' required to keep ie6 happy is only ever sent to ie6 users (may they rest in peace).

I think the whole GWT approach feels quite strange to JavaScript developers. As a pragmatist I can see the value in it for large applications, much as I love KnockoutJS (my personal favourite framework), it is not the ideal tool for apps with 1000s of lines of code.

Colin E.

@sindresorhus
Copy link
Member

You use > in front of the text block. More here.

Ok, got it, not as bad as I thought. That is actually quite useful. Since, as you say, you don't have to send all the IE junk to users that actually cares about which browser they're using.

Agreed. GWT is probably best fit for bigger apps than a todo app. But still, nice to have an example :)

@ColinEberhardt
Copy link
Member Author

@sindresorhus

FYI ... I have moved your comments to https://github.com/ColinEberhardt/todomvc/issues so that I can track them more easily, and make sure I do not miss anything.

@ColinEberhardt
Copy link
Member Author

@sindresorhus I think I am pretty happy with the GWT implementation now, I'll leave those few remaining issues open.

Let me know if you are happy to pull this code.

toggleAllCheckBox.setValue(totalTasks == completedTasks);
}

private void hideElement(Element element, boolean hide) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be called toggleElement, since it doesn't just hide it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really toggle it either! - perhaps showOrHideElement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@addyosmani
Copy link
Member

@sindresorhus how happy are you with the GWT implementation at the moment? We probably won't be merging this one in until we're closer to 1.0 so there's certainly time if more work needs to be done on this.

(thanks again @ColinEberhardt for being so responsive to our requests for tweaks) :)

@ColinEberhardt
Copy link
Member Author

@addyosmani @sindresorhus I'm more than happy to resolve this via override.css, which I have just done. The GWT Todo is up and running here:

http://colineberhardt.github.com/

Let me know when you plan to merge this in. An idea when your next release will be?

I plan to write up my experiences of developing the GWT version in a blog post next week, and will certainly highlight the benefits of the entire Todo MVC project.

Regards, Colin E.

@addyosmani
Copy link
Member

@ColinEberhardt Happy to RT that article when you publish, Colin. Thanks for taking the time on this again.

In terms of when our next release will be, early April might be a realistic first target for us. That'll give us a month to rewrite the existing apps, test and do as much additional documentation work or adoption of new frameworks as we might like.

@sindresorhus
Copy link
Member

What Addy said ;)

Though I don't see why you would need to add any CSS for this. Just use the <strong> for the number and a <span> for the pluralizeable word.

Tested it out, and it looks good.

One thing has changed since last time. We have decided that when a todo is edited, if it's empty or only contains whitespace (.trim() before check) when it's blur'ed, it should be removed instead of saved. This is to prevent empty todos.

In my opinion this is ready to be merged whenever you fix the above. But it's @addyosmani decision.
The next version, 1.0, will be released when it's done. Can't really say when. As this is an collaborated open source project, it depends a lot on the contributors having time to contribute. We want 1.0 to be a solid release with new apps, and all using the new theme and best practices. If you want the 1.0 sooner, feel free to take any unassigned issue and start working on it.

And again, thanks for contributing this app. I thinks it's very useful to have a GWT example in our collection.

@sindresorhus
Copy link
Member

👊 Damn you Addy, you always answer while I'm writing my comment :P

@addyosmani
Copy link
Member

Bwaha! It's what I do :)

@ColinEberhardt
Copy link
Member Author

One thing has changed since last time. We have decided that when a todo is edited, if it's empty or only contains whitespace (.trim() before check) when it's blur'ed, it should be removed instead of saved. This is to prevent empty todos.

^^^ Fixed :-)

@ColinEberhardt
Copy link
Member Author

Hey guys ... @addyosmani @sindresorhus @boushley

I have written a blog post about the TodoMVC project and my GWT implementation here:

http://www.scottlogic.co.uk/blog/colin/2012/03/developing-a-gwt-todomvc-application/

Regards, Colin E.

@addyosmani
Copy link
Member

Great write-up @ColinEberhardt!. Thanks for mentioning the project. Sure we'll be tweeting this :)

@sindresorhus
Copy link
Member

Yes, a good read :)

Found a couple of grammatical errors

@ColinEberhardt
Copy link
Member Author

Thanks, glad you liked it. I'll sort out the errors you spotted when I
am back in front of a PC.

There should be more software engineers as thorough as yourself!
Perfection is so rarely strived for or attained.

And I'm just as quilty of that as others ;-)
From: Sindre Sorhus
Sent: 12/03/2012 17:37
To: ColinEberhardt
Subject: Re: [todomvc] Added GWT implementation (#103)
Yes, a good read :)

Found a couple of grammatical
errors


Reply to this email directly or view it on GitHub:
#103 (comment)

@sindresorhus
Copy link
Member

Hi @ColinEberhardt hope you had a nice easter holiday :)

Would you be interested in updating the GWT app further?
We have some more changes to the index.html. See below.


We're working on standardazing the todo apps.
Would be great if you could do the following:

  • Use the latest template.
    The HTML changes are minimal: You can see the latest changes in this diff.
  • Make sure you follow the newly released App Specification.
    Please let me know if anything is unclear or could be improved. It's not field tested yet. The spine.js example, not yet landed, can be used as a reference.
  • This project uses tab indention. Use double-quotes in HTML and single-quotes in JS and CSS.

@ColinEberhardt
Copy link
Member Author

Hi @sindresorhus no problem, I'll try to sort something out later this week.

…, apart from 'toggle-all' button which is now plain-old HTML rather than a GWT CheckBox.

Uses common CSS file (../../assets/base.css)
Spaces => Tabs in Java code
@ColinEberhardt
Copy link
Member Author

Hi @sindresorhus & @addyosmani

I have updated the GWT implementation. Most of it was straightforward, apart from Sindre's funky re-styling of the toggle-all checkbox, which forced me to abandon the GWT CheckBox due to incompatible markup :-P

At least it demonstrates that you can throw out the high-level GWT framework and program directly against the DOM. You can see it up and running here:

http://colineberhardt.github.com/latest/gwt/

Diffs are a bit messy, should have committed the rename & space=>tab as separate units. Live and learn.

Regards, Colin E.

@sindresorhus
Copy link
Member

Look good :)

apart from Sindre's funky re-styling of the toggle-all checkbox, which forced me to abandon the GWT CheckBox due to incompatible markup :-P

I like tha funky :p Can you elaborate? The HTML for the checkbox is the same as it has always been, only the CSS changed.

  • If possible, can you rename the localStorage name from todo-gwt-state to todos-gwt
  • The border between todo items is missing

GitHub tip: You can append ?w=1 to a diff url to ignore whitespace changes. Comes in handy quite often.

@ColinEberhardt
Copy link
Member Author

I like tha funky :p Can you elaborate?

It's a GWT issue, because the CheckBox widget results in the generation of an input and a label, they have to wrap them in a containing element, which is a span in this case. As a result, the containing element has the id set. It was simpler to create the required markup manually than to mess around with app.css in this case.

Regarding the other two issues, I'll try to sort them out in the next couple of days.

Any idea when the next official release of TodoMVC will happen (and include GWT)?

@sindresorhus
Copy link
Member

Ok, got it. Thanks for explaining :)

We're shooting for a release in the end of May. As you can see from the milestone, there's a lot of stuff todo before that.

@sindresorhus
Copy link
Member

Any updates?

@ColinEberhardt
Copy link
Member Author

@sindresorhus sorry, nothing yet. I'm having a bit of a manic time at the moment!

@sindresorhus
Copy link
Member

We are releasing TodoMVC in 19 days. Would love to have the GWT example in there ;)

@ColinEberhardt
Copy link
Member Author

This has been on my personal todo list for a long time now ;-)

Finally found the time to sort these two issues out. Hopefully it is all OK now for GWT to be part of your upcoming release?

Good luck with it all!

@sindresorhus
Copy link
Member

Yes it is! Awesome work Colin. Thanks a lot for doing this :)

Landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants