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

Add a Lit TodoMVC app #251

Merged
merged 36 commits into from
Jun 28, 2023
Merged

Add a Lit TodoMVC app #251

merged 36 commits into from
Jun 28, 2023

Conversation

rictic
Copy link
Contributor

@rictic rictic commented Jun 21, 2023

This aims to fix #52

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I have a few comments and questions still.

I'm excited to see this in tree!

resources/tests.mjs Outdated Show resolved Hide resolved
resources/tests.mjs Outdated Show resolved Hide resolved
.prettierignore Show resolved Hide resolved
@flashdesignory
Copy link
Contributor

@rictic - lint check is failing. A good way to catch things is by running npm run format from Speedometer's root locally before pushing.
Speedometer has it's own prettier / eslint rules for consistency between workloads and to get as close as possible to Webkits style guide.

@rictic
Copy link
Contributor Author

rictic commented Jun 21, 2023

Huh, I think something is wrong with my prettier setup, as npm run format is making many changes to code not changed in this PR. Investigating.

@rictic
Copy link
Contributor Author

rictic commented Jun 21, 2023

I was definitely doing something wrong. This should now be properly formatted and passing eslint

@rniwa rniwa added the non-trivial change A change that affects benchmark results label Jun 22, 2023
@flashdesignory
Copy link
Contributor

@rictic - I noticed that you are defining the styles inside js, which is more idiomatic to lit, but doesn't let us manage consistency in the styles across todomvc apps. This workload also looks slightly different compared to the other ones.

I added an alternate approach with css-modules here (if approved):
#254

these could be installed as a local package and that could potentially work?
Note: these styles expect a slightly different dom structure.

Just posting here for thoughts.

rictic added 17 commits June 22, 2023 15:32
I had to `rm -rf ./node_modules/ ./package-lock.json` to get it to regen
This isn't part of the other todomvcs in Speedometer
This was formatted by a different prettier config, and I forgot to reformat every file when importing into Speedometer
Instead of firing events, we add methods onto the root app element to get the relevant parts of the private shadow DOM so that the test code can interact with them directly.

Added the callElementMethod method onto the Page class from WebKit#249
Currently, editing a todo (toggling it or changing its text) creates a new Todo object and splices it into the array. This is unnecessary work, if we instead make the `<todo-item>` take each field of the todo separately (common in Lit for leaf elements) then we can update it in place. This exercises more idiomatic Lit code (more template bindings) and less copying and splicing of the array of todos.
@flashdesignory
Copy link
Contributor

This build looks great and it's pretty easy to follow along and parsing the code 🚀 .

The only thing that threw me off was that the Todos class also handles route changes. I'd expect the app to listen for those and notify or set the filter in the Todos class., but that's maybe subjective.

Besides that, the only other difference I noticed was here the todos id is a simple int that increments, vs other implementations where we use uuid / nanoid.

This required some changes to the querySelectorAll implementation because the DOM is set up like:

* todo-app
  * todo-list
    * todo-item
      - .toggle
      - .destroy
    * todo-item
      - .toggle
      - .destroy
    * ...

The current implementation does the "selectAll" logic only at the final lowest level.

I implemented this as a generator function so that we don't do array allocation and copying at every level.
@flashdesignory
Copy link
Contributor

@bgrins @rniwa - Do we want to land this pr and deal with external css / constructable css mod later?
Don't want to block this pr and it feels like it's almost there.

Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

LGTM - potential alignment with a11y related updates could happen in a follow up pr.

@julienw
Copy link
Contributor

julienw commented Jun 27, 2023

Because this wasn't super clear for everybody, the lit code already uses socalled "constructable css" with its css template tag, and uses adoptedStyleSheets under the hood.
The related code is: https://github.com/lit/lit/blob/3711e6650a59966e5be8d92dd0abf053d9a50d32/packages/reactive-element/src/css-tag.ts

@rictic
Copy link
Contributor Author

rictic commented Jun 27, 2023

Yeah, expanding on that, the css template tag returns a CSSResult. We'd like it to just return a CSSStyleSheet, but we support older browser versions that didn't yet have constructible stylesheets. I could update this example to only use CSSStyleSheets, a LitElement's styles can take a CSSStyleSheet. I went with this approach because it's the most common way that Lit code is written in the wild.

@julienw
Copy link
Contributor

julienw commented Jun 28, 2023

Yeah, expanding on that, the css template tag returns a CSSResult. We'd like it to just return a CSSStyleSheet, but we support older browser versions that didn't yet have constructible stylesheets. I could update this example to only use CSSStyleSheets, a LitElement's styles can take a CSSStyleSheet. I went with this approach because it's the most common way that Lit code is written in the wild.

Yes, this sounds good to me! I just wanted other folks to know that this uses adoptedStyleSheets under the hood (for our current browser versions).

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me!
Just a few comments (including the one I left earlier about running npm i) and then we're good to go!

.prettierignore Outdated Show resolved Hide resolved
resources/benchmark-runner.mjs Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@@ -0,0 +1,53 @@
<!DOCTYPE html>
<html>
<head>
Copy link
Member

Choose a reason for hiding this comment

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

Can we use spaces instead of tabs like all other files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, done

It's hand authored, so it should be formatted and linted.
@flashdesignory flashdesignory merged commit 9e692e4 into WebKit:main Jun 28, 2023
@rictic rictic deleted the add-lit branch June 28, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-trivial change A change that affects benchmark results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lit-html todomvc demo
5 participants