TodoMVC in SAPUI5 #511
TodoMVC in SAPUI5 #511
Conversation
|
Thanks for your submission! We'll review and get back to you shortly. |
|
@schubertha I didn't know SAP had their own MVC framework. Very interesting. :) Could you check out our Contribution Guidelines and update the code to match the code style described there? |
|
Closing for lack of communication. @schubertha Feel free to reopen if you want to continue this. |
|
Hello everyone, sorry for not getting back to you earlier. I now managed to look into the contribution guidelines and update the sources as required (force-pushed into my fork). Can you re-open this very pull request or shall I create a new one? Regards, Harald |
|
Thanks for taking the time to update. @passy could you review the implementation for spec conformance? |
ghost
assigned
|
@schubertha Thanks for the update! Could you remove the Eclipse configs and other settings files? We want to keep the repository lean. |
| oRootView = sap.ui.view({ | ||
| type : sap.ui.core.mvc.ViewType.JS, | ||
| id : 'todoView', | ||
| viewName : 'todo.Todo' |
There was a problem hiding this comment.
No space before the colon:
oRootView = sap.ui.view({
type: sap.ui.core.mvc.ViewType.JS,
id: 'todoView',
viewName: 'todo.Todo'
...
|
Could you move the contents of We usually want the app to be self-contained and not reference external resources or CDNs. Is it possible to include the lib or what that cause licensing issues? |
| this.controls.push(todosRepeater); | ||
|
|
||
| // Counts open todos | ||
| todoCount = new sap.ui.commons.TextView('todo-count', { |
There was a problem hiding this comment.
There are some SAP CSS classes applied to this that override the base.css rules and make it look slightly different than the other apps. Could you look into that?
|
The 'clear completed' action is missing. |
|
This will take a bit. Please don't close the pull request - I'm working on it. Thanks! |
|
@schubertha Great! Looking forward to it. :) |
|
Almost there. |
|
Hey there, I think everything you asked for should be in now (with the exception of SAPUI5 itself which I can't push to Github for licensing reasons). Can you please check? One problem I still have is that the app doesn't seem to load if I try to test it via http://htmlpreview.github.io. Is there any other way I can ensure things are working properly? Best regards, |
|
Thanks for the update, @schubertha! I usually just check the branch out and run it locally. There are still some visual deviations from the template:
It's easiest to open a compliant example like the Angular or Backbone one and compare it to them. |
|
Hi, can you tell what browser you are using? Also, I didn't get your last point. Do I read it right supported browsers should NOT be mentioned? Then, how am I supposed to tell what browsers are supported? I hope I am not forced to support all browsers. The examples I saw usually also only work well with Cr and have issues on other browsers. Regards, |
I took that screenshot in Chrome on Linux.
As per spec all modern browsers + IE9 should be supported. |
|
Ok, I'll let you know when I am through with this. I'm afraid I can't test on Linux right now. On Windows, Chrome renders the page w/o difficulties (no padding issues, no inadequately sized checkmarks). Is this acceptable? BR, |
|
Hey Harald, that's fine. Tell me when you're done with the optimizations for the browsers you have access to and I can try to cover the Linux side. |
|
ping |
|
Still there... |
| // Text field for entering a new todo | ||
| newTodo = new todo.SmartTextField('new-todo', { | ||
| placeholder : 'What needs to be done?', | ||
| autofocus : !$.browser.msie && !$.browser.opera |
There was a problem hiding this comment.
Opera and IE will not show the placeholder in case you focus. By not focusing initially, you still see the placeholder...
|
I went through the code on my phone and will check the visuals on Linux
|
|
A few things regarding spec compliance:
|
|
Hi passy, I went through all your change comments. I also figured out I had an outdated Chrome version (v24). After updating, I also had the "padding at the top" issue. I fixed this now. Thanks in advance, |
That's wonderful! I'm going to go through it again tomorrow and I think we're getting close now. :) |
| sap.ui.commons.TextField.extend('todo.SmartTextField', { | ||
| metadata: { | ||
| properties: { | ||
| 'placeholder': 'string', |
|
If there are no items, the footer and the toggle button at the top should be hidden. |
|
And I just noticed that the font color in the footer is slightly different from the standard. |
|
All done. Thanks for checking! -Harald |
| jQuery.sap.declare('todo.formatters'); | ||
|
|
||
| todo.formatters = { | ||
| // Returns whether all todos are completed |
There was a problem hiding this comment.
This block is indented one tab too deep.
|
Thanks again, @schubertha! Code looks good to me now apart from that one nit-pick. I will go through the style discrepancies on Linux soon. |
|
@passy - many thanks! |
|
@schubertha As @sindresorhus found out, adding |
|
Also important, at least in my setup (OS X / Chrome), is |
|
@passy, I'm not sure I could follow. What issues are you trying to fix exactly? I can of course add the line you requested and see how this breaks the UI on Windows but is it his what you want me to do? |
|
@schubertha Here's what I'm seeing on OS X Chrome:
The |
|
Hi @passy, I have tweaked the CSS further and changed the order in which CSS gets loaded. I think the font family is correctly interpreted now. I also cannot find a difference in font aliasing between the SAPUI5 version and the Backbone.js version. Thanks for checking. |
|
@schubertha Thanks again for the update, I'm sorry I couldn't help out within the last weeks. I ran the example in a Windows VM to get a feeling for it and I see the same layout differences as on Linux. SAPUI Backbone 50% overlay Which configuration did you test it in? |
|
Thanks for the overlay. Good one. I'll look into those final differences. Just to confirm: you are really interested in pixel-perfect positioning of the texts in the footer and the icons - the rest is fine? |
|
Wow, this reminds me a certain kind of discussion I had with so many clients, but where I was arguing for the other side. :) In this case, however, a pixel-(near-)perfect position can be really helpful for the sake of comparison. Especially if this involves some extra hoops you have to jump through with a particular framework, because in some cases this actually is a project requirement. I'd really appreciate it if you could try to get as close as possible to the other apps. I will try to review the rest of the app tonight, so we can merge this straight away when the layout fixes are in. |
|
Sure, sure. I'm not arguing against it. I just wanted to hear your expectation. Working on further enhancements later today probably... |
|
@passy here is another update with a (to my view) pixel-perfect positioning: Hopefully this works for you :) |
|
@schubertha Excellent work! Thanks for being so patient and understanding. So glad we could finally merge this! 🍻 |
|
Merged via c2f442d |
|
@passy this is great! I'm happy to see this live now! 🍻 All the best, |
Hello everyone,
I work for SAP and have lately been looking into TodoMVC. The work you have started is really a great way to find out about and compare MVC JavaScript frameworks.
Now, at SAP we have developed a new JavaScript UI framework SAPUI5 that among other things features a MVC implementation as well as data binding.
I implemented the todo application and blogged about it on the SAP Community Network: http://scn.sap.com/community/developer-center/front-end/blog/2013/03/19/how-to-build-testable-sapui5-applications
While I made a copy of the code available on the SAP-owned code repository CodeExchange, I would also like to contribute a copy back to your GitHub project.
Please let me know what you think.
Best regards,
Harald Schubert