-
Notifications
You must be signed in to change notification settings - Fork 70
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
[OPTIONAL]: TodoMVC built with Web Components #249
Conversation
I suspect the Lit workload will cover similar technical ground, but we can always compare and contrast profiles. |
Yup, this is an alternate version to cover web components, in case we don't get the pr in time 😄 |
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.template.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-bottombar/todo-bottombar.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-bottombar/todo-bottombar.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
So interesting. With Lit workload (#251), browsers in the order of high score are: Chrome, Firefox, then Safari. With this workload, on the other hand, browsers in the order of high score are Safari, Chrome then Firefox with very little difference between Chrome & Firefox. This might be interesting / different enough workload to merit the inclusion based on that observation. |
There are several questions:
|
resources/tentative/todomvc/src/components/todo-list/todo-list.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-topbar/todo-topbar.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-item/todo-item.component.js
Outdated
Show resolved
Hide resolved
resources/tentative/todomvc/src/components/todo-list/todo-list.template.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work, this looks pretty solid! Also it's pretty nice to finally have an example that uses esm without a build step. IMO this may be the biggest interest for this workload.
I added a few comments, I didn't add them everywhere they apply by simplicity but of course they apply in all similar code.
I'm not approving just yet because I'd like to see the code with these changes again. Please add new commits for the changes so that the new review will be easy to do :-)
resources/todomvc/vanilla-examples/javascript-web-components/src/styles/app.constructable.js
Outdated
Show resolved
Hide resolved
...mvc/vanilla-examples/javascript-web-components/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
...mvc/vanilla-examples/javascript-web-components/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
resources/todomvc/vanilla-examples/javascript-web-components/src/styles/global.constructable.js
Show resolved
Hide resolved
...c/vanilla-examples/javascript-web-components/src/components/todo-item/todo-item.component.js
Outdated
Show resolved
Hide resolved
...c/vanilla-examples/javascript-web-components/src/components/todo-item/todo-item.component.js
Outdated
Show resolved
Hide resolved
resources/todomvc/vanilla-examples/javascript-web-components/src/data.js
Outdated
Show resolved
Hide resolved
resources/todomvc/vanilla-examples/javascript-web-components/src/index.js
Outdated
Show resolved
Hide resolved
…rc/components/todo-app/todo-app.component.js Co-authored-by: Julien Wajsberg <[email protected]>
…rc/components/todo-app/todo-app.component.js Co-authored-by: Julien Wajsberg <[email protected]>
…rc/components/todo-item/todo-item.component.js Co-authored-by: Julien Wajsberg <[email protected]>
this.htmlDirection = document.querySelector("html").getAttribute("dir") || "ltr"; | ||
this.shadow.host.setAttribute("dir", this.htmlDirection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote in my previous comments that the same comments were applying to other files :-) can you fix those too?
document.querySelector("html").getAttribute("dir")
=>document.dir
this.shadow.host
=>this
In all components
Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes 🤦 - thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fixes, looks good to me now
This pr is a simple implementation of the todo app using native web-components (with shadow dom)..
This is a "backup" solution in case we want something in the workloads that uses web components extensively.
Notes:
Scores:
Chrome:
Firefox:
Safari:
Edge:
@kara